Hi,
Here again for a new revision of the system-controller on Mobileye
EyeQ5. It deals with clk, reset and pinctrl.
Series dependencies are:
- We are based on v6.8-rc6.
- Both [1] and [0] series are needed, both are in mips-next [2].
- 4 patches from previous iteration have been applied to clk-next [3]
and dropped from this series. Those are needed for this to build
and test this series. Patches are:
- [PATCH v7 01/14] clk: fixed-factor: add optional accuracy support
- [PATCH v7 02/14] clk: fixed-factor: add fwname-based constructor functions
- [PATCH v7 03/14] dt-bindings: clock: mobileye,eyeq5-clk: add bindings
- [PATCH v7 04/14] dt-bindings: reset: mobileye,eyeq5-reset: add bindings
The patch adding the system-controller dt-bindings ("dt-bindings: soc:
mobileye: add EyeQ5 OLB system controller") is dependent on the three
controllers dt-bindings:
- dt-bindings: clock: mobileye,eyeq5-clk: add bindings
- dt-bindings: reset: mobileye,eyeq5-reset: add bindings
- dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings
Here is the patch list, split by subsystems. Pinctrl, clk and reset are
meant to be applicable without dependencies from external trees. They
apply fine on {clk,pinctrl}-next.
- pinctrl:
[PATCH v8 01/10] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings
[PATCH v8 05/10] pinctrl: eyeq5: add platform driver
- clk:
[PATCH v8 03/10] clk: eyeq5: add platform driver, and init routine at of_clk_init()
- reset:
[PATCH v8 04/10] reset: eyeq5: add platform driver
- MIPS:
[PATCH v8 02/10] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller
[PATCH v8 06/10] MAINTAINERS: Map OLB files to Mobileye SoCs
[PATCH v8 07/10] MIPS: mobileye: eyeq5: add OLB syscon node
[PATCH v8 08/10] MIPS: mobileye: eyeq5: use OLB clocks controller node
[PATCH v8 09/10] MIPS: mobileye: eyeq5: add OLB reset controller node
[PATCH v8 10/10] MIPS: mobileye: eyeq5: add pinctrl node & pinmux function nodes
Thanks all for the previous feedback!
Have a nice day,
Théo Lebrun
[0]: https://lore.kernel.org/lkml/[email protected]/
[1]: https://lore.kernel.org/linux-mips/[email protected]/
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/log/
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/log/?h=clk-next
Signed-off-by: Théo Lebrun <[email protected]>
---
Changes in v8:
- clk: drop two clk subsystem patches, clk dt-bindings and reset
dt-bindings (4 patches taken into clk-next).
- clk: Kconfig: fix help block indent.
- clk: fix #includes and whitespace.
- clk: add comment to explicit error conditions at start of probe.
- clk: fix error checking of devm_platform_ioremap_resource_byname() and
do not shadow the error.
- clk: if parsing registers fail, put error in clk rather than keeping
the EPROBE_DEFER set at init.
- clk: use dev_err_probe() on clk_hw_register_*() failure in probe.
- Link to v7: https://lore.kernel.org/r/[email protected]
Changes in v7:
- pinctrl: use dev_err_probe() and lower first letter of messages.
- Move all MAINTAINERS changes to a separate patch aimed at the MIPS
tree. This means clk/reset/pinctrl can take patches independently.
- Apply two Reviewed-by: Linus Walleij.
- Link to v6: https://lore.kernel.org/r/[email protected]
Changes in v6:
- bindings/clk: remove minItems in reg.
- bindings/reset: remove minItems in reg.
- bindings/syscon: unify quotes (use single quotes everywhere).
- bindings: apply three Reviewed-By tags from Krzysztof.
- Rebased onto v6.8-rc5. No related changes. Tested on EyeQ5 hardware.
- Link to v5: https://lore.kernel.org/r/[email protected]
Changes in v5:
- pinctrl: fix pin/offset distinction, add eq5p_pin_to_offset() helper,
rename eq5p_pin_offset_to_bank() to eq5p_pin_to_bank(), rename
eq5p_readl_bit() to eq5p_test_bit(), remove old <linux/mfd/syscon.h>
include, add defensive check in eq5p_test_bit().
- dt-bindings/MIPS: OLB example (dt-bindings) and devicetree: fix
ordering of nodes and properties, fix register casing.
- dt-bindings: add pin group node example to OLB dt-bindings.
- MIPS: squashed "MIPS: mobileye: eyeq5: add reset properties to UARTs"
into "MIPS: mobileye: eyeq5: add OLB reset controller node".
- MIPS: squashed "MIPS: mobileye: eyeq5: add pinctrl properties to UART
nodes" into "MIPS: mobileye: eyeq5: add pinctrl node & pinmux
function nodes".
- MIPS: rebased onto V7 of [0], meaning we now introduce the OLB syscon
node in DT in this series rather than modifying it.
- Apply Reviewed-by from Rob onto "dt-bindings: pinctrl:
mobileye,eyeq5-pinctrl: add bindings".
- dt-bindings: Drop "dt-bindings: pinctrl: allow pin controller device
without unit address".
- dt-bindings: I did NOT apply Krzysztof's Reviewed-By trailers from v3
as I am not sure he acked the changes made in V4.
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- Have the three drivers access MMIO directly rather than through the
syscon & regmap.
- pinctrl: Make the pin controller handle both banks using a single
instance.
- pinctrl/dt-bindings: Add if/else for each function, to strictly define
possible functions.
- clk: Changing to direct MMIO means we can use
clk_hw_register_divider_table_parent_hw() for the OSPI table-based
divider clock.
- Use builtin_platform_driver() for platform driver registering instead
of manual initcalls.
- reset: follow Philipp & Krzysztof's feedback:
- Use container_of() to get private struct.
- Use '_withlock' suffix instead of the underscore prefix.
- Use udelay() instead of the non-standard __udelay().
- Remove useless checks.
- Use mutex guards.
- Remove the ->reset() implementation.
- Use devres variants for kzalloc() and reset_controller_register().
- Other small changes following feedback from reviewers. dt-bindings
whitespace for pinctrl.yaml, fix pinctrl driver dt-bindings
description, improve clk driver commit header, etc.
- Link to v3: https://lore.kernel.org/r/[email protected]
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/[email protected]
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/[email protected]
---
Théo Lebrun (10):
dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings
dt-bindings: soc: mobileye: add EyeQ5 OLB system controller
clk: eyeq5: add platform driver, and init routine at of_clk_init()
reset: eyeq5: add platform driver
pinctrl: eyeq5: add platform driver
MAINTAINERS: Map OLB files to Mobileye SoCs
MIPS: mobileye: eyeq5: add OLB syscon node
MIPS: mobileye: eyeq5: use OLB clocks controller node
MIPS: mobileye: eyeq5: add OLB reset controller node
MIPS: mobileye: eyeq5: add pinctrl node & pinmux function nodes
.../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 242 +++++++++
.../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 94 ++++
MAINTAINERS | 8 +
.../{eyeq5-fixed-clocks.dtsi => eyeq5-clocks.dtsi} | 54 +-
arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi | 125 +++++
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 42 +-
drivers/clk/Kconfig | 11 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-eyeq5.c | 306 +++++++++++
drivers/pinctrl/Kconfig | 15 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-eyeq5.c | 577 +++++++++++++++++++++
drivers/reset/Kconfig | 12 +
drivers/reset/Makefile | 1 +
drivers/reset/reset-eyeq5.c | 342 ++++++++++++
15 files changed, 1792 insertions(+), 39 deletions(-)
---
base-commit: f48300386c909467ddd455ff619351214d94aa54
change-id: 20231023-mbly-clk-87ce5c241f08
Best regards,
--
Théo Lebrun <[email protected]>
Add documentation to describe the "Other Logic Block" syscon.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>
---
.../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 94 ++++++++++++++++++++++
1 file changed, 94 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..bcded7fb86dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
@@ -0,0 +1,94 @@
+# 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 <[email protected]>
+ - Théo Lebrun <[email protected]>
+ - Vladimir Kondratiev <[email protected]>
+
+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
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 1
+
+ ranges: true
+
+patternProperties:
+ '^clock-controller@[0-9a-f]+$':
+ $ref: /schemas/clock/mobileye,eyeq5-clk.yaml#
+
+ '^reset-controller@[0-9a-f]+$':
+ $ref: /schemas/reset/mobileye,eyeq5-reset.yaml#
+
+ '^pinctrl@[0-9a-f]+$':
+ $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
+
+required:
+ - compatible
+ - reg
+ - '#address-cells'
+ - '#size-cells'
+ - ranges
+
+additionalProperties: false
+
+examples:
+ - |
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ system-controller@e00000 {
+ compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
+ reg = <0x0 0xe00000 0x0 0x400>;
+ ranges = <0x0 0x0 0xe00000 0x400>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ reset: reset-controller@0 {
+ compatible = "mobileye,eyeq5-reset";
+ reg = <0x000 0x0c>, <0x200 0x34>, <0x120 0x04>;
+ reg-names = "d0", "d1", "d2";
+ #reset-cells = <2>;
+ };
+
+ clocks: clock-controller@2c {
+ compatible = "mobileye,eyeq5-clk";
+ reg = <0x02c 0x50>, <0x11c 0x04>;
+ reg-names = "plls", "ospi";
+ #clock-cells = <1>;
+ clocks = <&xtal>;
+ clock-names = "ref";
+ };
+
+ pinctrl: pinctrl@b0 {
+ compatible = "mobileye,eyeq5-pinctrl";
+ reg = <0x0b0 0x30>;
+
+ uart2_pins: uart2-pins {
+ function = "uart2";
+ pins = "PB8", "PB9";
+ };
+ };
+ };
+ };
--
2.44.0
We add the clock controller inside the OLB syscon region and remove
previous fixed devicetree nodes representing PLLs exposed by the clock
controller.
Signed-off-by: Théo Lebrun <[email protected]>
---
.../{eyeq5-fixed-clocks.dtsi => eyeq5-clocks.dtsi} | 54 +++++++---------------
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 11 ++++-
2 files changed, 26 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 e82d2a57f6da..1a65b43e13b1 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>;
@@ -106,6 +106,15 @@ olb: system-controller@e00000 {
ranges = <0x0 0x0 0xe00000 0x400>;
#address-cells = <1>;
#size-cells = <1>;
+
+ clocks: clock-controller@e0002c {
+ compatible = "mobileye,eyeq5-clk";
+ reg = <0x02c 0x50>, <0x11c 0x04>;
+ reg-names = "plls", "ospi";
+ #clock-cells = <1>;
+ clocks = <&xtal>;
+ clock-names = "ref";
+ };
};
gic: interrupt-controller@140000 {
--
2.44.0
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 <[email protected]>
---
drivers/reset/Kconfig | 12 ++
drivers/reset/Makefile | 1 +
drivers/reset/reset-eyeq5.c | 342 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 355 insertions(+)
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..f9d3935dd420
--- /dev/null
+++ b/drivers/reset/reset-eyeq5.c
@@ -0,0 +1,342 @@
+// 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/cleanup.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+
+/* Domain 0 register offsets */
+#define D0_SARCR0 (0x004)
+#define D0_SARCR1 (0x008)
+
+/* Domain 1 masks */
+#define D1_ACRP_PD_REQ BIT(0)
+#define D1_ACRP_ST_POWER_DOWN BIT(27)
+#define D1_ACRP_ST_ACTIVE BIT(29)
+
+/* Vendor-provided timeout 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];
+ void __iomem *base_d0;
+ void __iomem *base_d1;
+ void __iomem *base_d2;
+ struct reset_controller_dev rcdev;
+};
+
+#define rcdev_to_priv(rcdev) container_of(rcdev, struct eq5r_private, rcdev)
+
+static int eq5r_busy_wait_withlock(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++) {
+ val = readl(priv->base_d0 + D0_SARCR1);
+ val = !(val & BIT(offset));
+ if (val == assert)
+ return 0;
+ udelay(1);
+ }
+ break;
+ case 1:
+ mask = assert ? D1_ACRP_ST_POWER_DOWN : D1_ACRP_ST_ACTIVE;
+ for (i = 0; i < D1_TIMEOUT_POLL; i++) {
+ val = readl(priv->base_d1 + 4 * offset);
+ 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_withlock(struct eq5r_private *priv, u32 domain,
+ u32 offset)
+{
+ void __iomem *reg;
+
+ lockdep_assert_held(&priv->mutexes[domain]);
+
+ switch (domain) {
+ case 0:
+ reg = priv->base_d0 + D0_SARCR0;
+ writel(readl(reg) & ~BIT(offset), reg);
+ break;
+ case 1:
+ reg = priv->base_d1 + 4 * offset;
+ writel(readl(reg) | D1_ACRP_PD_REQ, reg);
+ break;
+ case 2:
+ reg = priv->base_d2;
+ writel(readl(reg) & ~BIT(offset), reg);
+ break;
+ default:
+ WARN_ON(1);
+ }
+}
+
+static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+ struct eq5r_private *priv = rcdev_to_priv(rcdev);
+ u32 offset = id & GENMASK(7, 0);
+ u32 domain = id >> 8;
+
+ dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
+
+ guard(mutex)(&priv->mutexes[domain]);
+ eq5r_assert_withlock(priv, domain, offset);
+ return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, true);
+}
+
+static void eq5r_deassert_withlock(struct eq5r_private *priv, u32 domain,
+ u32 offset)
+{
+ void __iomem *reg;
+
+ lockdep_assert_held(&priv->mutexes[domain]);
+
+ switch (domain) {
+ case 0:
+ reg = priv->base_d0 + D0_SARCR0;
+ writel(readl(reg) | BIT(offset), reg);
+ break;
+ case 1:
+ reg = priv->base_d1 + 4 * offset;
+ writel(readl(reg) & ~D1_ACRP_PD_REQ, reg);
+ break;
+ case 2:
+ reg = priv->base_d2;
+ writel(readl(reg) | BIT(offset), reg);
+ break;
+ default:
+ WARN_ON(1);
+ }
+}
+
+static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+ struct eq5r_private *priv = rcdev_to_priv(rcdev);
+ u32 offset = id & GENMASK(7, 0);
+ u32 domain = id >> 8;
+
+ dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset);
+
+ guard(mutex)(&priv->mutexes[domain]);
+ eq5r_deassert_withlock(priv, domain, offset);
+ return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, false);
+}
+
+static int eq5r_status(struct reset_controller_dev *rcdev, unsigned long id)
+{
+ struct eq5r_private *priv = rcdev_to_priv(rcdev);
+ u32 offset = id & GENMASK(7, 0);
+ u32 domain = id >> 8;
+ u32 val;
+
+ dev_dbg(rcdev->dev, "%u-%u: status request\n", domain, offset);
+
+ guard(mutex)(&priv->mutexes[domain]);
+
+ switch (domain) {
+ case 0:
+ val = readl(priv->base_d0 + D0_SARCR1);
+ return !(val & BIT(offset));
+ case 1:
+ val = readl(priv->base_d1 + 4 * offset);
+ return !(val & D1_ACRP_ST_ACTIVE);
+ case 2:
+ val = readl(priv->base_d2);
+ return !(val & BIT(offset));
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct reset_control_ops eq5r_ops = {
+ .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 eq5r_private *priv;
+ int ret, i;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->base_d0 = devm_platform_ioremap_resource_byname(pdev, "d0");
+ if (IS_ERR(priv->base_d0))
+ return PTR_ERR(priv->base_d0);
+
+ priv->base_d1 = devm_platform_ioremap_resource_byname(pdev, "d1");
+ if (IS_ERR(priv->base_d1))
+ return PTR_ERR(priv->base_d1);
+
+ priv->base_d2 = devm_platform_ioremap_resource_byname(pdev, "d2");
+ if (IS_ERR(priv->base_d2))
+ return PTR_ERR(priv->base_d2);
+
+ 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 = devm_reset_controller_register(dev, &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,
+ },
+};
+
+builtin_platform_driver(eq5r_driver);
--
2.44.0
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 <[email protected]>
---
drivers/clk/Kconfig | 11 ++
drivers/clk/Makefile | 1 +
drivers/clk/clk-eyeq5.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 318 insertions(+)
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 50af5fc7f570..c79b38f60b1b 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..85bf6f1c3fa3
--- /dev/null
+++ b/drivers/clk/clk-eyeq5.c
@@ -0,0 +1,306 @@
+// 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/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <dt-bindings/clock/mobileye,eyeq5-clk.h>
+
+/* In frac mode, it enables fractional noise canceling DAC. Else, no function. */
+#define PCSR0_DAC_EN BIT(0)
+/* Fractional or integer mode */
+#define PCSR0_DSM_EN BIT(1)
+#define PCSR0_PLL_EN BIT(2)
+/* All clocks output held at 0 */
+#define PCSR0_FOUTPOSTDIV_EN BIT(3)
+#define PCSR0_POST_DIV1 GENMASK(6, 4)
+#define PCSR0_POST_DIV2 GENMASK(9, 7)
+#define PCSR0_REF_DIV GENMASK(15, 10)
+#define PCSR0_INTIN GENMASK(27, 16)
+#define PCSR0_BYPASS BIT(28)
+/* Bits 30..29 are reserved */
+#define PCSR0_PLL_LOCKED BIT(31)
+
+#define PCSR1_RESET BIT(0)
+#define PCSR1_SSGC_DIV GENMASK(4, 1)
+/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
+#define PCSR1_SPREAD GENMASK(9, 5)
+#define PCSR1_DIS_SSCG BIT(10)
+/* Down-spread or center-spread */
+#define PCSR1_DOWN_SPREAD BIT(11)
+#define PCSR1_FRAC_IN GENMASK(31, 12)
+
+static struct clk_hw_onecell_data *eq5c_clk_data;
+
+struct eq5c_pll {
+ int index;
+ const char *name;
+ u32 reg; /* next 8 bytes are r0 and r1 */
+};
+
+/* 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 = 0x00, },
+ { .index = EQ5C_PLL_PER, .name = "pll-per", .reg = 0x30, },
+};
+
+static const struct eq5c_pll eq5c_plls[] = {
+ { .index = EQ5C_PLL_VMP, .name = "pll-vmp", .reg = 0x08, },
+ { .index = EQ5C_PLL_PMA, .name = "pll-pma", .reg = 0x10, },
+ { .index = EQ5C_PLL_VDI, .name = "pll-vdi", .reg = 0x18, },
+ { .index = EQ5C_PLL_DDR0, .name = "pll-ddr0", .reg = 0x20, },
+ { .index = EQ5C_PLL_PCI, .name = "pll-pci", .reg = 0x28, },
+ { .index = EQ5C_PLL_PMAC, .name = "pll-pmac", .reg = 0x38, },
+ { .index = EQ5C_PLL_MPC, .name = "pll-mpc", .reg = 0x40, },
+ { .index = EQ5C_PLL_DDR1, .name = "pll-ddr1", .reg = 0x48, },
+};
+
+#define EQ5C_OSPI_DIV_CLK_NAME "div-ospi"
+#define EQ5C_OSPI_DIV_WIDTH 4
+
+#define EQ5C_NB_CLKS (ARRAY_SIZE(eq5c_early_plls) + ARRAY_SIZE(eq5c_plls) + 1)
+
+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 */
+};
+
+static int eq5c_pll_parse_registers(u32 r0, u32 r1, unsigned long *mult,
+ unsigned long *div, unsigned long *acc)
+{
+ if (r0 & PCSR0_BYPASS) {
+ *mult = 1;
+ *div = 1;
+ *acc = 0;
+ return 0;
+ }
+
+ if (!(r0 & PCSR0_PLL_LOCKED))
+ return -EINVAL;
+
+ *mult = FIELD_GET(PCSR0_INTIN, r0);
+ *div = FIELD_GET(PCSR0_REF_DIV, r0);
+ if (r0 & PCSR0_FOUTPOSTDIV_EN)
+ *div *= FIELD_GET(PCSR0_POST_DIV1, r0) * FIELD_GET(PCSR0_POST_DIV2, r0);
+
+ /* Fractional mode, in 2^20 (0x100000) parts. */
+ if (r0 & PCSR0_DSM_EN) {
+ *div *= 0x100000;
+ *mult = *mult * 0x100000 + FIELD_GET(PCSR1_FRAC_IN, r1);
+ }
+
+ if (!*mult || !*div)
+ return -EINVAL;
+
+ /* Spread spectrum. */
+ if (!(r1 & (PCSR1_RESET | 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(PCSR1_SPREAD, r1);
+
+ *acc = spread * 500000;
+ if (r1 & PCSR1_DOWN_SPREAD) {
+ /*
+ * Downspreading: the central frequency is half a
+ * spread lower.
+ */
+ *mult *= 2000 - spread;
+ *div *= 2000;
+ }
+ } else {
+ *acc = 0;
+ }
+
+ return 0;
+}
+
+static int eq5c_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ void __iomem *base_plls, *base_ospi;
+ struct clk_hw *hw;
+ int i;
+
+ /* Return potential error from eq5c_init(). */
+ if (IS_ERR(eq5c_clk_data))
+ return PTR_ERR(eq5c_clk_data);
+ /* Return an error if eq5c_init() did not get called. */
+ else if (!eq5c_clk_data)
+ return -EINVAL;
+
+ base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
+ if (IS_ERR(base_plls))
+ return PTR_ERR(base_plls);
+
+ base_ospi = devm_platform_ioremap_resource_byname(pdev, "ospi");
+ if (IS_ERR(base_ospi))
+ return PTR_ERR(base_ospi);
+
+ 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;
+
+ r0 = readl(base_plls + pll->reg);
+ r1 = readl(base_plls + pll->reg + sizeof(r0));
+
+ ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
+ if (ret) {
+ dev_warn(dev, "failed parsing state of %s\n", pll->name);
+ eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
+ 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_probe(dev, PTR_ERR(hw), "failed registering %s\n",
+ pll->name);
+ }
+
+ hw = clk_hw_register_divider_table_parent_hw(dev, EQ5C_OSPI_DIV_CLK_NAME,
+ eq5c_clk_data->hws[EQ5C_PLL_PER], 0,
+ base_ospi, 0, EQ5C_OSPI_DIV_WIDTH, 0,
+ eq5c_ospi_div_table, NULL);
+ eq5c_clk_data->hws[EQ5C_DIV_OSPI] = hw;
+ if (IS_ERR(hw))
+ dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
+ EQ5C_OSPI_DIV_CLK_NAME);
+
+ 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)
+{
+ void __iomem *base_plls, *base_ospi;
+ int index_plls, index_ospi;
+ 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);
+
+ index_plls = of_property_match_string(np, "reg-names", "plls");
+ if (index_plls < 0) {
+ ret = index_plls;
+ goto err;
+ }
+
+ index_ospi = of_property_match_string(np, "reg-names", "ospi");
+ if (index_ospi < 0) {
+ ret = index_ospi;
+ goto err;
+ }
+
+ base_plls = of_iomap(np, index_plls);
+ base_ospi = of_iomap(np, index_ospi);
+ if (!base_plls || !base_ospi) {
+ ret = -ENODEV;
+ 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;
+
+ r0 = readl(base_plls + pll->reg);
+ r1 = readl(base_plls + pll->reg + sizeof(r0));
+
+ ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
+ if (ret) {
+ pr_warn("failed parsing state of %s\n", pll->name);
+ eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
+ 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);
+}
+
+CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
--
2.44.0
Add all OLB related (system-controller) files to the MOBILEYE MIPS SOCS
entry.
Signed-off-by: Théo Lebrun <[email protected]>
---
This does not follow the standard of adding MAINTAINERS entries with
file additions. We avoid dependencies inbetween maintainer trees. The
MOBILEYE MIPS SOCS is added by [0] series. It is currently in
mips-next [1].
[0]: https://lore.kernel.org/lkml/[email protected]/
[1]: https://lore.kernel.org/lkml/[email protected]/
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index d2a6a3937e88..73bd2851eb4c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14793,10 +14793,18 @@ M: Gregory CLEMENT <[email protected]>
M: Théo Lebrun <[email protected]>
L: [email protected]
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/
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
MODULE SUPPORT
--
2.44.0
Add dt-schema type bindings for the Mobileye EyeQ5 pin controller.
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>
---
.../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 242 +++++++++++++++++++++
1 file changed, 242 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..5f00604bf48c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
@@ -0,0 +1,242 @@
+# 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 the two pin banks of the system. It belongs
+ to a system-controller block called OLB.
+
+ Pin control is about bias (pull-down, pull-up), drive strength and muxing. Pin
+ muxing supports two functions for each pin: first is GPIO, second is
+ pin-dependent.
+
+ Pins and groups are bijective.
+
+maintainers:
+ - Grégory Clement <[email protected]>
+ - Théo Lebrun <[email protected]>
+ - Vladimir Kondratiev <[email protected]>
+
+$ref: pinctrl.yaml#
+
+properties:
+ compatible:
+ enum:
+ - mobileye,eyeq5-pinctrl
+
+ reg:
+ maxItems: 1
+
+patternProperties:
+ "-pins?$":
+ type: object
+ description: Pin muxing configuration.
+ $ref: pinmux-node.yaml#
+ additionalProperties: false
+ properties:
+ pins: true
+ function:
+ enum: [gpio,
+ # Bank A
+ timer0, timer1, timer2, timer5, uart0, uart1, can0, can1, spi0,
+ spi1, refclk0,
+ # Bank B
+ timer3, timer4, timer6, uart2, can2, spi2, spi3, mclk0]
+ bias-disable: true
+ bias-pull-down: true
+ bias-pull-up: true
+ drive-strength: true
+ required:
+ - pins
+ - function
+ allOf:
+ - if:
+ properties:
+ function:
+ const: gpio
+ then:
+ properties:
+ pins:
+ items: # PA0 - PA28, PB0 - PB22
+ pattern: '^(P(A|B)1?[0-9]|PA2[0-8]|PB2[0-2])$'
+ - if:
+ properties:
+ function:
+ const: timer0
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA0, PA1]
+ - if:
+ properties:
+ function:
+ const: timer1
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA2, PA3]
+ - if:
+ properties:
+ function:
+ const: timer2
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA4, PA5]
+ - if:
+ properties:
+ function:
+ const: timer5
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA6, PA7, PA8, PA9]
+ - if:
+ properties:
+ function:
+ const: uart0
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA10, PA11]
+ - if:
+ properties:
+ function:
+ const: uart1
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA12, PA13]
+ - if:
+ properties:
+ function:
+ const: can0
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA14, PA15]
+ - if:
+ properties:
+ function:
+ const: can1
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA16, PA17]
+ - if:
+ properties:
+ function:
+ const: spi0
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA18, PA19, PA20, PA21, PA22]
+ - if:
+ properties:
+ function:
+ const: spi1
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA23, PA24, PA25, PA26, PA27]
+ - if:
+ properties:
+ function:
+ const: refclk0
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PA28]
+ - if:
+ properties:
+ function:
+ const: timer3
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PB0, PB1]
+ - if:
+ properties:
+ function:
+ const: timer4
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PB2, PB3]
+ - if:
+ properties:
+ function:
+ const: timer6
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PB4, PB5, PB6, PB7]
+ - if:
+ properties:
+ function:
+ const: uart2
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PB8, PB9]
+ - if:
+ properties:
+ function:
+ const: can2
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PB10, PB11]
+ - if:
+ properties:
+ function:
+ const: spi2
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PB12, PB13, PB14, PB15, PB16]
+ - if:
+ properties:
+ function:
+ const: spi3
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PB17, PB18, PB19, PB20, PB21]
+ - if:
+ properties:
+ function:
+ const: mclk0
+ then:
+ properties:
+ pins:
+ items:
+ enum: [PB22]
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
--
2.44.0
The OLB ("Other Logic Block") is a syscon region hosting the clock,
reset and pin controllers. It contains registers such as I2C speed mode
that need to be accessible by other nodes.
Signed-off-by: Théo Lebrun <[email protected]>
---
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 6cc5980e2fa1..e82d2a57f6da 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -100,6 +100,14 @@ uart2: serial@a00000 {
clock-names = "uartclk", "apb_pclk";
};
+ olb: system-controller@e00000 {
+ compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
+ reg = <0 0xe00000 0x0 0x400>;
+ ranges = <0x0 0x0 0xe00000 0x400>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ };
+
gic: interrupt-controller@140000 {
compatible = "mti,gic";
reg = <0x0 0x140000 0x0 0x20000>;
--
2.44.0
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.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/pinctrl/Kconfig | 15 ++
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-eyeq5.c | 577 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 593 insertions(+)
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..ffab4e7f5618
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-eyeq5.c
@@ -0,0 +1,577 @@
+// 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.
+ *
+ * We use eq5p_ as prefix, as-in "EyeQ5 Pinctrl", but way shorter.
+ *
+ * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
+ */
+#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/seq_file.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+
+struct eq5p_pinctrl {
+ struct pinctrl_desc desc;
+ void __iomem *base;
+};
+
+struct eq5p_function {
+ const char *name;
+ const char * const *groups;
+ unsigned int ngroups;
+};
+
+enum eq5p_bank {
+ EQ5P_BANK_A,
+ EQ5P_BANK_B,
+
+ EQ5P_BANK_COUNT,
+};
+
+enum eq5p_regs {
+ EQ5P_PD,
+ EQ5P_PU,
+ EQ5P_DS_LOW,
+ EQ5P_DS_HIGH,
+ EQ5P_IOCR,
+
+ EQ5P_REG_COUNT,
+};
+
+static const unsigned int eq5p_regs[EQ5P_BANK_COUNT][EQ5P_REG_COUNT] = {
+ [EQ5P_BANK_A] = {0x010, 0x014, 0x020, 0x024, 0x000},
+ [EQ5P_BANK_B] = {0x018, 0x01C, 0x028, 0x02C, 0x004},
+};
+
+/*
+ * Comments to the right of each pin are the "signal name" in the datasheet.
+ */
+static const struct pinctrl_pin_desc eq5p_pins[] = {
+ /* 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 */
+
+#define EQ5P_PIN_OFFSET_BANK_B 29
+
+ /* Bank B */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 0, "PB0"), /* B0_TIMER3_CK */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 1, "PB1"), /* B1_TIMER3_EOC */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 2, "PB2"), /* B2_TIMER4_CK */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 3, "PB3"), /* B3_TIMER4_EOC */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 4, "PB4"), /* B4_TIMER6_EXT_INCAP1 */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 5, "PB5"), /* B5_TIMER6_EXT_INCAP2 */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 6, "PB6"), /* B6_TIMER6_EXT_OUTCMP1 */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 7, "PB7"), /* B7_TIMER6_EXT_OUTCMP2 */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 8, "PB8"), /* B8_UART_2_TX */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 9, "PB9"), /* B9_UART_2_RX */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 10, "PB10"), /* B10_CAN_2_TX */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 11, "PB11"), /* B11_CAN_2_RX */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 12, "PB12"), /* B12_SPI_2_DO */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 13, "PB13"), /* B13_SPI_2_DI */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 14, "PB14"), /* B14_SPI_2_CK */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 15, "PB15"), /* B15_SPI_2_CS0 */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 16, "PB16"), /* B16_SPI_2_CS1 */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 17, "PB17"), /* B17_SPI_3_DO */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 18, "PB18"), /* B18_SPI_3_DI */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 19, "PB19"), /* B19_SPI_3_CK */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 20, "PB20"), /* B20_SPI_3_CS0 */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 21, "PB21"), /* B21_SPI_3_CS1 */
+ PINCTRL_PIN(EQ5P_PIN_OFFSET_BANK_B + 22, "PB22"), /* B22_MCLK0 */
+};
+
+static const char * const gpio_groups[] = {
+ /* Bank A */
+ "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",
+
+ /* Bank B */
+ "PB0", "PB1", "PB2", "PB3", "PB4", "PB5", "PB6", "PB7", "PB8", "PB9",
+ "PB10", "PB11", "PB12", "PB13", "PB14", "PB15", "PB16", "PB17", "PB18",
+ "PB19", "PB20", "PB21", "PB22",
+};
+
+/* Groups of functions on bank A */
+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 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[] = {
+ /* GPIO having a fixed index is depended upon, see GPIO_FUNC_SELECTOR. */
+ FUNCTION("gpio", gpio_groups),
+#define GPIO_FUNC_SELECTOR 0
+
+ /* Bank A functions */
+ 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),
+
+ /* Bank B functions */
+ 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),
+};
+
+static void eq5p_update_bits(const struct eq5p_pinctrl *pctrl,
+ enum eq5p_bank bank, enum eq5p_regs reg,
+ u32 mask, u32 val)
+{
+ void __iomem *ptr = pctrl->base + eq5p_regs[bank][reg];
+
+ writel((readl(ptr) & ~mask) | (val & mask), ptr);
+}
+
+static bool eq5p_test_bit(const struct eq5p_pinctrl *pctrl,
+ enum eq5p_bank bank, enum eq5p_regs reg, int offset)
+{
+ u32 val = readl(pctrl->base + eq5p_regs[bank][reg]);
+
+ if (WARN_ON(offset > 31))
+ return false;
+
+ return (val & BIT(offset)) != 0;
+}
+
+static enum eq5p_bank eq5p_pin_to_bank(unsigned int pin)
+{
+ if (pin < EQ5P_PIN_OFFSET_BANK_B)
+ return EQ5P_BANK_A;
+ else
+ return EQ5P_BANK_B;
+}
+
+static unsigned int eq5p_pin_to_offset(unsigned int pin)
+{
+ if (pin < EQ5P_PIN_OFFSET_BANK_B)
+ return pin;
+ else
+ return pin - EQ5P_PIN_OFFSET_BANK_B;
+}
+
+static int eq5p_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ return ARRAY_SIZE(eq5p_pins);
+}
+
+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 pin,
+ unsigned long *config);
+
+static void eq5p_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
+ struct seq_file *s,
+ unsigned int pin)
+{
+ struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+ const char *pin_name = pctrl->desc.pins[pin].name;
+ unsigned int offset = eq5p_pin_to_offset(pin);
+ enum eq5p_bank bank = eq5p_pin_to_bank(pin);
+ const char *func_name, *bias;
+ unsigned long ds_config;
+ u32 drive_strength;
+ bool pd, pu;
+ int i, j;
+
+ /*
+ * First, let's get the function name. All pins have only two functions:
+ * GPIO (IOCR == 0) and something else (IOCR == 1).
+ */
+ if (!eq5p_test_bit(pctrl, bank, EQ5P_IOCR, offset)) {
+ func_name = eq5p_functions[GPIO_FUNC_SELECTOR].name;
+ } else {
+ func_name = NULL;
+ for (i = 0; i < ARRAY_SIZE(eq5p_functions); i++) {
+ if (i == GPIO_FUNC_SELECTOR)
+ continue;
+
+ for (j = 0; j < eq5p_functions[i].ngroups; j++) {
+ /* Groups and pins are the same thing for us. */
+ const char *x = eq5p_functions[i].groups[j];
+
+ if (strcmp(x, pin_name) == 0) {
+ func_name = eq5p_functions[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. */
+ pd = eq5p_test_bit(pctrl, bank, EQ5P_PD, offset);
+ pu = eq5p_test_bit(pctrl, bank, EQ5P_PU, offset);
+ 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, pin, &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)
+{
+ return ARRAY_SIZE(eq5p_functions);
+}
+
+static const char *eq5p_pinmux_get_function_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ return eq5p_functions[selector].name;
+}
+
+static int eq5p_pinmux_get_function_groups(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const char * const **groups,
+ unsigned int *num_groups)
+{
+ *groups = eq5p_functions[selector].groups;
+ *num_groups = eq5p_functions[selector].ngroups;
+ return 0;
+}
+
+static int eq5p_pinmux_set_mux(struct pinctrl_dev *pctldev,
+ unsigned int func_selector, unsigned int pin)
+{
+ struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+ const char *func_name = eq5p_functions[func_selector].name;
+ const char *group_name = pctldev->desc->pins[pin].name;
+ bool is_gpio = func_selector == GPIO_FUNC_SELECTOR;
+ unsigned int offset = eq5p_pin_to_offset(pin);
+ enum eq5p_bank bank = eq5p_pin_to_bank(pin);
+ u32 mask, val;
+
+ dev_dbg(pctldev->dev, "%s: func=%s group=%s\n", __func__, func_name,
+ group_name);
+
+ mask = BIT(offset);
+ val = is_gpio ? 0 : U32_MAX;
+ eq5p_update_bits(pctrl, bank, EQ5P_IOCR, mask, val);
+ return 0;
+}
+
+static int eq5p_pinmux_gpio_request_enable(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range,
+ unsigned int pin)
+{
+ /* Pin numbers and group selectors are the same thing in our case. */
+ return eq5p_pinmux_set_mux(pctldev, GPIO_FUNC_SELECTOR, pin);
+}
+
+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 pin,
+ unsigned long *config)
+{
+ enum pin_config_param param = pinconf_to_config_param(*config);
+ struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+ unsigned int offset = eq5p_pin_to_offset(pin);
+ enum eq5p_bank bank = eq5p_pin_to_bank(pin);
+ u32 val_ds, arg = 0;
+ bool pd, pu;
+
+ pd = eq5p_test_bit(pctrl, bank, EQ5P_PD, offset);
+ pu = eq5p_test_bit(pctrl, bank, EQ5P_PU, offset);
+
+ 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) {
+ val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_HIGH]);
+ offset -= 32;
+ } else {
+ val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_LOW]);
+ }
+ arg = (val_ds >> offset) & 0b11;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ *config = pinconf_to_config_packed(param, arg);
+ return 0;
+}
+
+static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev,
+ unsigned int pin, u32 arg)
+{
+ struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+ unsigned int offset = eq5p_pin_to_offset(pin);
+ enum eq5p_bank bank = eq5p_pin_to_bank(pin);
+ unsigned int reg;
+ u32 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 = EQ5P_DS_HIGH;
+ offset -= 32;
+ } else {
+ reg = EQ5P_DS_LOW;
+ }
+
+ mask = 0b11 << offset;
+ val = arg << offset;
+ eq5p_update_bits(pctrl, bank, reg, mask, val);
+ return 0;
+}
+
+static int eq5p_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+ unsigned long *configs, unsigned int num_configs)
+{
+ struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+ const char *pin_name = pctldev->desc->pins[pin].name;
+ unsigned int offset = eq5p_pin_to_offset(pin);
+ enum eq5p_bank bank = eq5p_pin_to_bank(pin);
+ struct device *dev = pctldev->dev;
+ u32 val = BIT(offset);
+ unsigned int i;
+
+ 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, "pin=%s bias_disable\n", pin_name);
+
+ eq5p_update_bits(pctrl, bank, EQ5P_PD, val, 0);
+ eq5p_update_bits(pctrl, bank, EQ5P_PU, val, 0);
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ dev_dbg(dev, "pin=%s bias_pull_down arg=%u\n",
+ pin_name, arg);
+
+ if (arg == 0) /* cannot connect to GND */
+ return -ENOTSUPP;
+
+ eq5p_update_bits(pctrl, bank, EQ5P_PD, val, val);
+ eq5p_update_bits(pctrl, bank, EQ5P_PU, val, 0);
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_UP:
+ dev_dbg(dev, "pin=%s bias_pull_up arg=%u\n",
+ pin_name, arg);
+
+ if (arg == 0) /* cannot connect to VDD */
+ return -ENOTSUPP;
+
+ eq5p_update_bits(pctrl, bank, EQ5P_PD, val, 0);
+ eq5p_update_bits(pctrl, bank, EQ5P_PU, val, val);
+ break;
+
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ dev_dbg(dev, "pin=%s drive_strength arg=%u\n",
+ pin_name, arg);
+
+ eq5p_pinconf_set_drive_strength(pctldev, pin, arg);
+ break;
+
+ default:
+ dev_err(dev, "Unsupported pinconf %u\n", param);
+ return -ENOTSUPP;
+ }
+ }
+
+ 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,
+};
+
+static int eq5p_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pinctrl_dev *pctldev;
+ struct eq5p_pinctrl *pctrl;
+ int ret;
+
+ pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+ if (!pctrl)
+ return -ENOMEM;
+
+ pctrl->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(pctrl->base))
+ return PTR_ERR(pctrl->base);
+
+ pctrl->desc.name = dev_name(dev);
+ pctrl->desc.pins = eq5p_pins;
+ pctrl->desc.npins = ARRAY_SIZE(eq5p_pins);
+ 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)
+ return dev_err_probe(dev, ret, "failed registering pinctrl device\n");
+
+ ret = pinctrl_enable(pctldev);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed enabling pinctrl device\n");
+
+ return 0;
+}
+
+static const struct of_device_id eq5p_match[] = {
+ { .compatible = "mobileye,eyeq5-pinctrl" },
+ {},
+};
+
+static struct platform_driver eq5p_driver = {
+ .driver = {
+ .name = "eyeq5-pinctrl",
+ .of_match_table = eq5p_match,
+ },
+ .probe = eq5p_probe,
+};
+
+builtin_platform_driver(eq5p_driver);
--
2.44.0
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.
Add reset phandles to UART nodes.
Signed-off-by: Théo Lebrun <[email protected]>
---
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 1a65b43e13b1..76935f237ab5 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 {
@@ -107,6 +110,13 @@ olb: system-controller@e00000 {
#address-cells = <1>;
#size-cells = <1>;
+ reset: reset-controller@e00000 {
+ compatible = "mobileye,eyeq5-reset";
+ reg = <0x000 0x0c>, <0x200 0x34>, <0x120 0x04>;
+ reg-names = "d0", "d1", "d2";
+ #reset-cells = <2>;
+ };
+
clocks: clock-controller@e0002c {
compatible = "mobileye,eyeq5-clk";
reg = <0x02c 0x50>, <0x11c 0x04>;
--
2.44.0
Pins on this platform have two functions: GPIO or something-else. We
create function nodes for each something-else based on functions.
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 <[email protected]>
---
arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi | 125 ++++++++++++++++++++++++++++
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 13 +++
2 files changed, 138 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..42acda13e57a
--- /dev/null
+++ b/arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi
@@ -0,0 +1,125 @@
+// 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.
+ */
+
+&pinctrl {
+ 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";
+ };
+ 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 76935f237ab5..8d4f65ec912d 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 {
@@ -125,6 +131,11 @@ clocks: clock-controller@e0002c {
clocks = <&xtal>;
clock-names = "ref";
};
+
+ pinctrl: pinctrl@e000b0 {
+ compatible = "mobileye,eyeq5-pinctrl";
+ reg = <0x0b0 0x30>;
+ };
};
gic: interrupt-controller@140000 {
@@ -149,3 +160,5 @@ timer {
};
};
};
+
+#include "eyeq5-pins.dtsi"
--
2.44.0
On Tue, 27 Feb 2024 15:55:23 +0100, Théo Lebrun wrote:
> Add documentation to describe the "Other Logic Block" syscon.
>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 94 ++++++++++++++++++++++
> 1 file changed, 94 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@2c: False schema does not allow {'compatible': ['mobileye,eyeq5-clk'], 'reg': [[44, 80], [284, 4]], 'reg-names': ['plls', 'ospi'], '#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@0: False schema does not allow {'compatible': ['mobileye,eyeq5-reset'], 'reg': [[0, 12], [512, 52], [288, 4]], 'reg-names': ['d0', 'd1', 'd2'], '#reset-cells': [[2]]}
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/soc/system-controller@e00000/reset-controller@0: failed to match any schema with compatible: ['mobileye,eyeq5-reset']
Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/soc/system-controller@e00000/clock-controller@2c: failed to match any schema with compatible: ['mobileye,eyeq5-clk']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
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.
On Tue, Feb 27, 2024 at 03:55:24PM +0100, 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
If you wrap 'It' to the next line, overall text will look better.
> 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
Ditto for 'the'
> UARTs.
..
> +config COMMON_CLK_EYEQ5
> + bool "Clock driver for the Mobileye EyeQ5 platform"
> + depends on OF
Since it's a functional dependency, why not allow compile test without OF being
enabled?
> + 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.
..
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
+ errno.h (yes, you need both)
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
+ overflow.h
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
..
> +struct eq5c_pll {
> + int index;
Index can be negative? Any comment about this case?
> + const char *name;
> + u32 reg; /* next 8 bytes are r0 and r1 */
Not sure this comments gives any clarification to a mere reader of the code.
Perhaps you want to name this as reg64 (at least it will show that you have
8 bytes, but I have no clue what is the semantic relationship between r0 and
r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> +};
..
> +static int eq5c_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
It's used only once. Why not just use dev->of_node there?
> + void __iomem *base_plls, *base_ospi;
> + struct clk_hw *hw;
> + int i;
> +
> + /* Return potential error from eq5c_init(). */
> + if (IS_ERR(eq5c_clk_data))
> + return PTR_ERR(eq5c_clk_data);
> + /* Return an error if eq5c_init() did not get called. */
> + else if (!eq5c_clk_data)
Redundant 'else'
> + return -EINVAL;
I didn't get. If eq5c_init() was finished successfully, why do you need to
seems repeat what it already done? What did I miss?
> + base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
> + if (IS_ERR(base_plls))
> + return PTR_ERR(base_plls);
> +
> + base_ospi = devm_platform_ioremap_resource_byname(pdev, "ospi");
> + if (IS_ERR(base_ospi))
> + return PTR_ERR(base_ospi);
> +
> + 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;
> +
> + r0 = readl(base_plls + pll->reg);
> + r1 = readl(base_plls + pll->reg + sizeof(r0));
> +
> + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> + if (ret) {
> + dev_warn(dev, "failed parsing state of %s\n", pll->name);
> + eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
> + 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_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> + pll->name);
Missed return statement?
> + }
> +
> + hw = clk_hw_register_divider_table_parent_hw(dev, EQ5C_OSPI_DIV_CLK_NAME,
> + eq5c_clk_data->hws[EQ5C_PLL_PER], 0,
> + base_ospi, 0, EQ5C_OSPI_DIV_WIDTH, 0,
> + eq5c_ospi_div_table, NULL);
> + eq5c_clk_data->hws[EQ5C_DIV_OSPI] = hw;
> + if (IS_ERR(hw))
> + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> + EQ5C_OSPI_DIV_CLK_NAME);
Ditto.
> + return 0;
> +}
> +static void __init eq5c_init(struct device_node *np)
> +{
> + void __iomem *base_plls, *base_ospi;
> + int index_plls, index_ospi;
> + int i, ret;
Why is i signed?
> + 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);
> + index_plls = of_property_match_string(np, "reg-names", "plls");
> + if (index_plls < 0) {
> + ret = index_plls;
> + goto err;
> + }
Better pattern is to avoid the output pollution in the error case. Hence
ret = of_property_match_string(np, "reg-names", "plls");
if (ret < 0)
goto err;
index_plls = ret;
> + index_ospi = of_property_match_string(np, "reg-names", "ospi");
> + if (index_ospi < 0) {
> + ret = index_ospi;
> + goto err;
> + }
Ditto.
> + base_plls = of_iomap(np, index_plls);
> + base_ospi = of_iomap(np, index_ospi);
> + if (!base_plls || !base_ospi) {
> + ret = -ENODEV;
> + 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;
> +
> + r0 = readl(base_plls + pll->reg);
> + r1 = readl(base_plls + pll->reg + sizeof(r0));
> +
> + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> + if (ret) {
> + pr_warn("failed parsing state of %s\n", pll->name);
> + eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
> + 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",
%pe ?
> + pll->name, PTR_ERR(hw));
Is the error not critical? Is it fine? How is it supposed to work at such
circumstances?
> + }
> +
> + 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);
> +}
> +
> +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
--
With Best Regards,
Andy Shevchenko
On Tue, Feb 27, 2024 at 03:55:25PM +0100, 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.
..
The inclusion block is a semi-random. Please, follow IWYU principle.
+ array_size.h
+ bits.h
+ bug.h
+ container_of.h
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
+ device.h
+ err.h
+ io.h
+ lockdep.h
+ mod_devicetable.h
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
Are all of them in use?
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
+ types.h
..
> +/* Vendor-provided timeout values. D1 has a long timeout because of LBIST. */
> +#define D0_TIMEOUT_POLL 10
> +#define D1_TIMEOUT_POLL 40000
We use units as suffixes. The above doesn't tell magnitude of timeouts.
Also constants like USEC_PER_MSEC are useful to make code robust.
..
> + unsigned int val, mask;
> + int i;
> +
> + lockdep_assert_held(&priv->mutexes[domain]);
> +
> + switch (domain) {
> + case 0:
> + for (i = 0; i < D0_TIMEOUT_POLL; i++) {
> + val = readl(priv->base_d0 + D0_SARCR1);
> + val = !(val & BIT(offset));
> + if (val == assert)
> + return 0;
> + udelay(1);
> + }
> + break;
See what iopoll.h gives you.
> + case 1:
> + mask = assert ? D1_ACRP_ST_POWER_DOWN : D1_ACRP_ST_ACTIVE;
> + for (i = 0; i < D1_TIMEOUT_POLL; i++) {
> + val = readl(priv->base_d1 + 4 * offset);
> + if (val & mask)
> + return 0;
> + udelay(1);
> + }
Ditto.
> + 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_withlock(struct eq5r_private *priv, u32 domain,
> + u32 offset)
> +{
> + void __iomem *reg;
> +
> + lockdep_assert_held(&priv->mutexes[domain]);
> +
> + switch (domain) {
> + case 0:
> + reg = priv->base_d0 + D0_SARCR0;
> + writel(readl(reg) & ~BIT(offset), reg);
> + break;
> + case 1:
> + reg = priv->base_d1 + 4 * offset;
> + writel(readl(reg) | D1_ACRP_PD_REQ, reg);
> + break;
> + case 2:
> + reg = priv->base_d2;
> + writel(readl(reg) & ~BIT(offset), reg);
> + break;
> + default:
> + WARN_ON(1);
break;
> + }
> +}
> +
> +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> + struct eq5r_private *priv = rcdev_to_priv(rcdev);
> + u32 offset = id & GENMASK(7, 0);
> + u32 domain = id >> 8;
> +
> + dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
> +
> + guard(mutex)(&priv->mutexes[domain]);
> + eq5r_assert_withlock(priv, domain, offset);
> + return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, true);
> +}
> +
> +static void eq5r_deassert_withlock(struct eq5r_private *priv, u32 domain,
> + u32 offset)
> +{
> + void __iomem *reg;
> +
> + lockdep_assert_held(&priv->mutexes[domain]);
> +
> + switch (domain) {
> + case 0:
> + reg = priv->base_d0 + D0_SARCR0;
> + writel(readl(reg) | BIT(offset), reg);
> + break;
> + case 1:
> + reg = priv->base_d1 + 4 * offset;
> + writel(readl(reg) & ~D1_ACRP_PD_REQ, reg);
> + break;
> + case 2:
> + reg = priv->base_d2;
> + writel(readl(reg) | BIT(offset), reg);
> + break;
> + default:
> + WARN_ON(1);
break;
> + }
> +}
..
> +static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> + struct eq5r_private *priv = rcdev_to_priv(rcdev);
> + u32 offset = id & GENMASK(7, 0);
> + u32 domain = id >> 8;
Perhaps
u32 offset = (id & GENMASK(7, 0)) >> 0;
u32 domain = (id & GENMASK(31, 8)) >> 8;
for better understanding the split?
> + dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset);
> +
> + guard(mutex)(&priv->mutexes[domain]);
> + eq5r_deassert_withlock(priv, domain, offset);
> + return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, false);
withlock is not usual naming pattern, "locked" is shorter and widely used.
> +}
> +
> +static int eq5r_status(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> + struct eq5r_private *priv = rcdev_to_priv(rcdev);
> + u32 offset = id & GENMASK(7, 0);
> + u32 domain = id >> 8;
Ditto.
> + u32 val;
> +
> + dev_dbg(rcdev->dev, "%u-%u: status request\n", domain, offset);
> +
> + guard(mutex)(&priv->mutexes[domain]);
> +
> + switch (domain) {
> + case 0:
> + val = readl(priv->base_d0 + D0_SARCR1);
> + return !(val & BIT(offset));
> + case 1:
> + val = readl(priv->base_d1 + 4 * offset);
> + return !(val & D1_ACRP_ST_ACTIVE);
> + case 2:
> + val = readl(priv->base_d2);
> + return !(val & BIT(offset));
> + default:
> + return -EINVAL;
> + }
> +}
..
> + struct eq5r_private *priv;
> + int ret, i;
Why is i signed?
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->base_d0 = devm_platform_ioremap_resource_byname(pdev, "d0");
> + if (IS_ERR(priv->base_d0))
> + return PTR_ERR(priv->base_d0);
> +
> + priv->base_d1 = devm_platform_ioremap_resource_byname(pdev, "d1");
> + if (IS_ERR(priv->base_d1))
> + return PTR_ERR(priv->base_d1);
> +
> + priv->base_d2 = devm_platform_ioremap_resource_byname(pdev, "d2");
> + if (IS_ERR(priv->base_d2))
> + return PTR_ERR(priv->base_d2);
> +
> + 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;
It's better to use device_set_node().
> + 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]);
Please, use corresponding hweightXX() API.
> + ret = devm_reset_controller_register(dev, &priv->rcdev);
> + if (ret) {
> + dev_err(dev, "Failed registering reset controller: %d\n", ret);
> + return ret;
return dev_err_probe(...);
> + }
> +
> + return 0;
> +}
..
> +static struct platform_driver eq5r_driver = {
> + .probe = eq5r_probe,
> + .driver = {
> + .name = "eyeq5-reset",
> + .of_match_table = eq5r_match_table,
> + },
> +};
> +
Unneeded blank line.
> +builtin_platform_driver(eq5r_driver);
--
With Best Regards,
Andy Shevchenko
On Tue, Feb 27, 2024 at 03:55:26PM +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.
..
> +config PINCTRL_EYEQ5
> + bool "Mobileye EyeQ5 pinctrl driver"
Can't be a module?
> + depends on OF
It's even not needed for this software as far as I can tell from the code.
> + 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.
..
> +#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/seq_file.h>
Semi-random list of the inclusions. Please, fix it.
While doing that, group out pinctrl/* ones as it's done in other drivers.
> +#include "core.h"
> +#include "pinctrl-utils.h"
..
> +struct eq5p_function {
> + const char *name;
> + const char * const *groups;
> + unsigned int ngroups;
> +};
We have struct pinfunction, use it instead.
..
> +static const char * const gpio_groups[] = {
> + /* Bank A */
> + "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",
For all arrays like this, please split them on 4/8/10/16 items per line as it's
much easier to count and refer by index when reading the code.
> + /* Bank B */
> + "PB0", "PB1", "PB2", "PB3", "PB4", "PB5", "PB6", "PB7", "PB8", "PB9",
> + "PB10", "PB11", "PB12", "PB13", "PB14", "PB15", "PB16", "PB17", "PB18",
> + "PB19", "PB20", "PB21", "PB22",
> +};
..
> +#define FUNCTION(a, b) { .name = a, .groups = b, .ngroups = ARRAY_SIZE(b) }
Use PINCTRL_PINFUNCTION() instead.
..
> +static bool eq5p_test_bit(const struct eq5p_pinctrl *pctrl,
> + enum eq5p_bank bank, enum eq5p_regs reg, int offset)
> +{
> + u32 val = readl(pctrl->base + eq5p_regs[bank][reg]);
> + if (WARN_ON(offset > 31))
> + return false;
When this condition can be true?
> + return (val & BIT(offset)) != 0;
> +}
..
> +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long *config);
Can't you avoid forward declarations?
..
> + if (!eq5p_test_bit(pctrl, bank, EQ5P_IOCR, offset)) {
What's wrong with positive conditional?
> + } else {
> + }
..
> +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,
ifdef is missing for these... But the question is, isn't these a default when
OF is in use?
> +};
..
> + dev_dbg(pctldev->dev, "%s: func=%s group=%s\n", __func__, func_name,
> + group_name);
Drop __func__ from all debug messages. With Dynamic Debug enabled (which is
often the case) we can do it at run-time).
..
> + mask = BIT(offset);
> + val = is_gpio ? 0 : U32_MAX;
I think you meant something else (semantically) than U32_MAX.
Perhaps GENMASK(31, 0)?
..
> +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long *config)
> +{
> + enum pin_config_param param = pinconf_to_config_param(*config);
> + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int offset = eq5p_pin_to_offset(pin);
> + enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> + u32 val_ds, arg = 0;
What's arg assignment for?
> + bool pd, pu;
> +
> + pd = eq5p_test_bit(pctrl, bank, EQ5P_PD, offset);
> + pu = eq5p_test_bit(pctrl, bank, EQ5P_PU, offset);
> +
> + 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) {
> + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_HIGH]);
> + offset -= 32;
> + } else {
> + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_LOW]);
> + }
I'm wondering why you can't use your helpers before multiplication?
> + arg = (val_ds >> offset) & 0b11;
GENMASK(1, 0)
> + break;
> + default:
> + return -ENOTSUPP;
> + }
> +
> + *config = pinconf_to_config_packed(param, arg);
> + return 0;
> +}
..
> +static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev,
> + unsigned int pin, u32 arg)
> +{
> + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int offset = eq5p_pin_to_offset(pin);
> + enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> + unsigned int reg;
> + u32 mask, val;
> +
> + if (arg > 3) {
Magic number.
> + dev_err(pctldev->dev, "Unsupported drive strength: %u\n", arg);
> + return -EINVAL;
> + }
> +
> + offset *= 2; /* two bits per pin */
> +
> + if (offset >= 32) {
> + reg = EQ5P_DS_HIGH;
> + offset -= 32;
> + } else {
> + reg = EQ5P_DS_LOW;
> + }
> + mask = 0b11 << offset;
> + val = arg << offset;
> + eq5p_update_bits(pctrl, bank, reg, mask, val);
Similar comments as per previous function.
> + return 0;
> +}
..
> +static const struct of_device_id eq5p_match[] = {
> + { .compatible = "mobileye,eyeq5-pinctrl" },
> + {},
No comma in the terminator entry.
> +};
No MODULE_DEVICE_TABLE()?
> +static struct platform_driver eq5p_driver = {
> + .driver = {
> + .name = "eyeq5-pinctrl",
> + .of_match_table = eq5p_match,
> + },
> + .probe = eq5p_probe,
> +};
> +
Unneeded blank line.
> +builtin_platform_driver(eq5p_driver);
--
With Best Regards,
Andy Shevchenko
Hello Andy,
Thanks for the review! I'll be skipping straight forward comments.
On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> > support for other platforms from Mobileye.
[...]
> > +config COMMON_CLK_EYEQ5
> > + bool "Clock driver for the Mobileye EyeQ5 platform"
>
> > + depends on OF
>
> Since it's a functional dependency, why not allow compile test without OF being
> enabled?
I'd do this then:
depends on OF || COMPILE_TEST
Which is better than removing the depend line. I wouldn't want the
kernel to build fine with OF=n even though we need it. OK for you?
>
> > + 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.
[...]
> > +struct eq5c_pll {
> > + int index;
>
> Index can be negative? Any comment about this case?
No it cannot. I did not care much because structs of this type are only
defined in the following static const table, using constants from
dt-bindings header.
I'll change to unsigned int.
>
> > + const char *name;
> > + u32 reg; /* next 8 bytes are r0 and r1 */
>
> Not sure this comments gives any clarification to a mere reader of the code.
> Perhaps you want to name this as reg64 (at least it will show that you have
> 8 bytes, but I have no clue what is the semantic relationship between r0 and
> r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
Clocks are defined by two 32-bit registers. We only store the first
register offset because they always follow each other.
I like the reg64 name and will remove the comment. This straight forward
code is found in the rest of the code, I don't think it is anything
hard to understand (ie does not need a comment):
u32 r0 = readl(base_plls + pll->reg);
u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
[...]
> > + return -EINVAL;
>
> I didn't get. If eq5c_init() was finished successfully, why do you need to
> seems repeat what it already done? What did I miss?
The key here is that eq5c_init() iterates on eq5c_early_plls[] while
eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
commit message:
> 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.
Doing everything in eq5c_init() is not clean because we expect all new
clock provider drivers to be standard platform drivers. Doing
everything from a platform driver probe doesn't work because some
clocks are required earlier than platform bus init. We therefore do a
mix.
This has been approved by Stephen Boyd in this email:
https://lore.kernel.org/lkml/[email protected]/
[...]
> > + base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
> > + if (IS_ERR(base_plls))
> > + return PTR_ERR(base_plls);
> > +
> > + base_ospi = devm_platform_ioremap_resource_byname(pdev, "ospi");
> > + if (IS_ERR(base_ospi))
> > + return PTR_ERR(base_ospi);
> > +
> > + 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;
> > +
> > + r0 = readl(base_plls + pll->reg);
> > + r1 = readl(base_plls + pll->reg + sizeof(r0));
> > +
> > + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> > + if (ret) {
> > + dev_warn(dev, "failed parsing state of %s\n", pll->name);
> > + eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
> > + 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_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> > + pll->name);
>
> Missed return statement?
No, we still try to register all clocks even if one failed. I guess we
can call this being optimistic.
[...]
> > +static void __init eq5c_init(struct device_node *np)
> > +{
> > + void __iomem *base_plls, *base_ospi;
> > + int index_plls, index_ospi;
> > + int i, ret;
>
> Why is i signed?
No reason, will be changed to unsigned int.
[...]
> > + 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",
>
> %pe ?
>
> > + pll->name, PTR_ERR(hw));
>
> Is the error not critical? Is it fine? How is it supposed to work at such
> circumstances?
It is a critical error, the system will stop working in a few
milliseconds. :-) This is different from probe and it should indeed
return the error.
Thanks for the review Andy.
Have a nice day,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello,
I won't be answering to straight forward comments.
On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 03:55:25PM +0100, 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.
>
> ...
>
> The inclusion block is a semi-random. Please, follow IWYU principle.
>
> + array_size.h
> + bits.h
> + bug.h
> + container_of.h
>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
>
> + device.h
> + err.h
> + io.h
> + lockdep.h
>
> + mod_devicetable.h
>
> > +#include <linux/mutex.h>
>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
>
> Are all of them in use?
>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
>
> + types.h
I'm adding yours + errno.h + init.h (for THIS_MODULE) + slab.h (for GFP
flags). I'm removing unused of_address.h and of_device.h.
delay.h will be removed and iopoll.h will be added based on changes
following your comments.
[...]
> > +static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > + struct eq5r_private *priv = rcdev_to_priv(rcdev);
> > + u32 offset = id & GENMASK(7, 0);
> > + u32 domain = id >> 8;
>
> Perhaps
>
> u32 offset = (id & GENMASK(7, 0)) >> 0;
> u32 domain = (id & GENMASK(31, 8)) >> 8;
>
> for better understanding the split?
Do the additional zero-bit-shift and GENMASK() help understanding? My
brain needs time to parse them to then notice they do nothing and
simplify the code in my head, back to the original version.
I personally like the simplest version (the original one). But otherwise
FIELD_GET() with two globally-defined masks could be a solution as
well. I still prefer the original version better. Less symbols, less
complexity.
[...]
> > + struct eq5r_private *priv;
> > + int ret, i;
>
> Why is i signed?
No reason, will switch to unsigned int.
>
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->base_d0 = devm_platform_ioremap_resource_byname(pdev, "d0");
> > + if (IS_ERR(priv->base_d0))
> > + return PTR_ERR(priv->base_d0);
> > +
> > + priv->base_d1 = devm_platform_ioremap_resource_byname(pdev, "d1");
> > + if (IS_ERR(priv->base_d1))
> > + return PTR_ERR(priv->base_d1);
> > +
> > + priv->base_d2 = devm_platform_ioremap_resource_byname(pdev, "d2");
> > + if (IS_ERR(priv->base_d2))
> > + return PTR_ERR(priv->base_d2);
> > +
> > + 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;
>
> It's better to use device_set_node().
I don't see how device_set_node() can help? It works on struct device
pointers. Here priv->rcdev is a reset_controller_dev struct. There are
no users of device_set_node() in drivers/reset/.
>
> > + 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]);
>
> Please, use corresponding hweightXX() API.
Noted. I did not find this keyword even though I searched quite a bit
for it. "popcount" sounds more logical to me. :-)
Thanks for the review Andy!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello,
On Tue Feb 27, 2024 at 7:14 PM CET, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 03:55:26PM +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.
>
> ...
>
> > +config PINCTRL_EYEQ5
> > + bool "Mobileye EyeQ5 pinctrl driver"
>
> Can't be a module?
It theory it could, I however do not see why that would be done. Pinctrl
is essential to the platform capabilities. The platform is an embedded
one and performance-oriented; boot-time is important and no user will
ever want to load pinctrl as a module.
>
> > + depends on OF
>
> It's even not needed for this software as far as I can tell from the code.
Indeed looks like it. Will try that out and remove the dependency if it
works as expected.
[...]
> > +#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/seq_file.h>
>
> Semi-random list of the inclusions. Please, fix it.
> While doing that, group out pinctrl/* ones as it's done in other drivers.
Here is my new list:
#include <linux/array_size.h>
#include <linux/bits.h>
#include <linux/bug.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/io.h>
#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/pinctrl/pinconf-generic.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/pinctrl/pinmux.h>
#include "core.h"
#include "pinctrl-utils.h"
[...]
> > +static bool eq5p_test_bit(const struct eq5p_pinctrl *pctrl,
> > + enum eq5p_bank bank, enum eq5p_regs reg, int offset)
> > +{
> > + u32 val = readl(pctrl->base + eq5p_regs[bank][reg]);
>
> > + if (WARN_ON(offset > 31))
> > + return false;
>
> When this condition can be true?
If there is a bug in the code. Defensive programming.
There is this subtle conversion of pin numbers => offset inside of a
bank. If one function forgets doing this then eq5p_test_bit() gets
called with a pin number.
In this GPIO series I fixed such a bug in a 10 year old driver:
https://lore.kernel.org/lkml/[email protected]/
The whole "if it can happen it will happen" mantra. We'll get a warning
in the logs using pinctrl-eyeq5.
>
> > + return (val & BIT(offset)) != 0;
> > +}
>
> ...
>
> > +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > + unsigned long *config);
>
> Can't you avoid forward declarations?
Yes, will do so.
>
> ...
>
> > + if (!eq5p_test_bit(pctrl, bank, EQ5P_IOCR, offset)) {
>
> What's wrong with positive conditional?
Nothing. In my mind GPIO was first, other was second. Will change.
>
>
> > + } else {
>
> > + }
>
> ...
>
> > +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,
>
> ifdef is missing for these... But the question is, isn't these a default when
> OF is in use?
Doesn't look like it is. In drivers/pinctrl/devicetree.c:
static int dt_to_map_one_config(struct pinctrl *p,
struct pinctrl_dev *hog_pctldev,
const char *statename,
struct device_node *np_config)
{
// ...
/*
* Call pinctrl driver to parse device tree node, and
* generate mapping table entries
*/
ops = pctldev->desc->pctlops;
if (!ops->dt_node_to_map) {
dev_err(p->dev, "pctldev %s doesn't support DT\n",
dev_name(pctldev->dev));
return -ENODEV;
}
// ...
}
And I see nowhere that puts a value if ->dt_node_to_map is empty.
For dt_free_map, it is an optional value. If the field is NULL nothing
is done. See dt_free_map() in the same file.
[...]
> > + mask = BIT(offset);
> > + val = is_gpio ? 0 : U32_MAX;
>
> I think you meant something else (semantically) than U32_MAX.
> Perhaps GENMASK(31, 0)?
To me the semantic of U32_MAX is the same. I see where you are coming
from. A better alternative however would be:
mask = BIT(offset);
val = is_gpio ? 0 : mask;
That way the desire is clear and the code is simpler.
>
> ...
>
> > +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > + unsigned long *config)
> > +{
> > + enum pin_config_param param = pinconf_to_config_param(*config);
> > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + unsigned int offset = eq5p_pin_to_offset(pin);
> > + enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> > + u32 val_ds, arg = 0;
>
> What's arg assignment for?
No reason indeed. Will remove the assignment.
>
> > + bool pd, pu;
> > +
> > + pd = eq5p_test_bit(pctrl, bank, EQ5P_PD, offset);
> > + pu = eq5p_test_bit(pctrl, bank, EQ5P_PU, offset);
> > +
> > + 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) {
> > + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_HIGH]);
> > + offset -= 32;
> > + } else {
> > + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_LOW]);
> > + }
>
> I'm wondering why you can't use your helpers before multiplication?
I'm unsure what helpers you are talking about?
If the question is about why multiply before if-condition: I feel like
multiplying first allows having the if condition be "offset >= 32".
That explicits why we readl HIGH vs LOW regs.
[...]
>
> > +static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev,
> > + unsigned int pin, u32 arg)
> > +{
> > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + unsigned int offset = eq5p_pin_to_offset(pin);
> > + enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> > + unsigned int reg;
> > + u32 mask, val;
> > +
> > + if (arg > 3) {
>
> Magic number.
Would 0b11 explicit why? The value is two bits wide, so 0 thru 3.
>
> > + dev_err(pctldev->dev, "Unsupported drive strength: %u\n", arg);
> > + return -EINVAL;
> > + }
> > +
> > + offset *= 2; /* two bits per pin */
> > +
> > + if (offset >= 32) {
> > + reg = EQ5P_DS_HIGH;
> > + offset -= 32;
> > + } else {
> > + reg = EQ5P_DS_LOW;
> > + }
>
> > + mask = 0b11 << offset;
> > + val = arg << offset;
> > + eq5p_update_bits(pctrl, bank, reg, mask, val);
>
> Similar comments as per previous function.
So GENMASK(1, 0) rather than 0b11. Or GENMASK(offset+1, offset).
Something else?
>
> > + return 0;
> > +}
>
> ...
>
> > +static const struct of_device_id eq5p_match[] = {
> > + { .compatible = "mobileye,eyeq5-pinctrl" },
> > + {},
>
> No comma in the terminator entry.
>
> > +};
>
> No MODULE_DEVICE_TABLE()?
It is an oversight. Will be added.
Thanks for the review Andy.
Have a nice day,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Feb 27, 2024 at 3:55 PM Théo Lebrun <[email protected]> wrote:
> Add dt-schema type bindings for the Mobileye EyeQ5 pin controller.
>
> Reviewed-by: Rob Herring <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Théo Lebrun <[email protected]>
Patch applied!
Let's start applying stuff so we get down the depth of the patch stacks.
Yours,
Linus Walleij
On Tue, Feb 27, 2024 at 3:55 PM Théo Lebrun <[email protected]> wrote:
> Pins on this platform have two functions: GPIO or something-else. We
> create function nodes for each something-else based on functions.
>
> 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 <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Feb 28, 2024 at 03:33:29PM +0100, Th?o Lebrun wrote:
> On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Th?o Lebrun wrote:
[...]
> > > + depends on OF
> >
> > Since it's a functional dependency, why not allow compile test without OF
> > being enabled?
>
> I'd do this then:
>
> depends on OF || COMPILE_TEST
>
> Which is better than removing the depend line. I wouldn't want the
> kernel to build fine with OF=n even though we need it. OK for you?
Yes!
[...]
> > > + u32 reg; /* next 8 bytes are r0 and r1 */
> >
> > Not sure this comments gives any clarification to a mere reader of the code.
> > Perhaps you want to name this as reg64 (at least it will show that you have
> > 8 bytes, but I have no clue what is the semantic relationship between r0 and
> > r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
>
> Clocks are defined by two 32-bit registers. We only store the first
> register offset because they always follow each other.
> I like the reg64 name and will remove the comment. This straight forward
> code is found in the rest of the code, I don't think it is anything
> hard to understand (ie does not need a comment):
>
> u32 r0 = readl(base_plls + pll->reg);
> u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h)
can be used in this case? It will be much better overall and be aligned with
reg64 name.
[...]
> > I didn't get. If eq5c_init() was finished successfully, why do you need to
> > seems repeat what it already done? What did I miss?
>
> The key here is that eq5c_init() iterates on eq5c_early_plls[] while
> eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
> commit message:
>
> > 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.
>
> Doing everything in eq5c_init() is not clean because we expect all new
> clock provider drivers to be standard platform drivers. Doing
> everything from a platform driver probe doesn't work because some
> clocks are required earlier than platform bus init. We therefore do a
> mix.
Am I missing something or these two pieces are using the same IO resources?
This looks like a lot of code duplication without clear benefit. Perhaps
you can have a helper?
> This has been approved by Stephen Boyd in this email:
> https://lore.kernel.org/lkml/[email protected]/
OK!
[...]
> > > + eq5c_clk_data->hws[pll->index] = hw;
> > > + if (IS_ERR(hw))
> >
> > > + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> > > + pll->name);
> >
> > Missed return statement?
>
> No, we still try to register all clocks even if one failed. I guess we
> can call this being optimistic.
But how critical these clocks are? I believe we should panic it we have no
critical calls be available. Otherwise, why '_err_'? Shouldn't be dev_warn()?
--
With Best Regards,
Andy Shevchenko
On Wed, Feb 28, 2024 at 06:04:47PM +0100, Th?o Lebrun wrote:
> On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Th?o Lebrun wrote:
..
> > > + u32 offset = id & GENMASK(7, 0);
> > > + u32 domain = id >> 8;
> >
> > Perhaps
> >
> > u32 offset = (id & GENMASK(7, 0)) >> 0;
> > u32 domain = (id & GENMASK(31, 8)) >> 8;
> >
> > for better understanding the split?
>
> Do the additional zero-bit-shift and GENMASK() help understanding? My
> brain needs time to parse them to then notice they do nothing and
> simplify the code in my head, back to the original version.
In my opinion yes, as you exactly showing the split.
But. The better is to use FIELD_GET().
> I personally like the simplest version (the original one). But otherwise
> FIELD_GET() with two globally-defined masks could be a solution as
> well.
Oh, yes, that's what just came to my mind without even looking here.
> I still prefer the original version better. Less symbols, less
> complexity.
[...]
> > > + priv->rcdev.of_node = np;
> >
> > It's better to use device_set_node().
>
> I don't see how device_set_node() can help? It works on struct device
> pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> no users of device_set_node() in drivers/reset/.
No users doesn't mean it's good. The API is relatively "new" and takes
care of two things:
1) it uses agnostic interface;
2) it doesn't require any firmware node direct dereference.
The 2) is most important here as allows us to refactor (firmware node) code
in the future.
> > > + priv->rcdev.of_reset_n_cells = 2;
> > > + priv->rcdev.of_xlate = eq5r_of_xlate;
However, ideally these should be also translated to use fwnode as IIO did,
for example.
..
> > > + priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
> >
> > Please, use corresponding hweightXX() API.
>
> Noted. I did not find this keyword even though I searched quite a bit
> for it. "popcount" sounds more logical to me. :-)
Hmm... But it's fundamental, it's called Hamming weight.
https://en.wikipedia.org/wiki/Hamming_weight
--
With Best Regards,
Andy Shevchenko
On Wed, Feb 28, 2024 at 07:15:12PM +0100, Th?o Lebrun wrote:
> On Tue Feb 27, 2024 at 7:14 PM CET, Andy Shevchenko wrote:
> > On Tue, Feb 27, 2024 at 03:55:26PM +0100, Th?o Lebrun wrote:
..
> > > + bool "Mobileye EyeQ5 pinctrl driver"
> >
> > Can't be a module?
>
> It theory it could, I however do not see why that would be done. Pinctrl
> is essential to the platform capabilities. The platform is an embedded
> one and performance-oriented; boot-time is important and no user will
> ever want to load pinctrl as a module.
I can argue. The modularization can give a better granularity in the exactly
embedded world when the memory resource (flash/RAM) is limited or fragmented
(for one or another reason). Having less weighty kernel at boot makes it smaller
to fit, for example, faster read only memory block which is not so uncommon.
The rule of thumb is to make modules if, otherwise, it's not so critical for
the boot process (and even for some cases we still may have it done as a module
with help of deferred probe mechanism).
[...]
> > > +#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/seq_file.h>
> >
> > Semi-random list of the inclusions. Please, fix it.
> > While doing that, group out pinctrl/* ones as it's done in other drivers.
>
> Here is my new list:
>
> #include <linux/array_size.h>
> #include <linux/bits.h>
> #include <linux/bug.h>
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/errno.h>
> #include <linux/io.h>
> #include <linux/mod_devicetable.h>
> #include <linux/platform_device.h>
> #include <linux/seq_file.h>
> #include <linux/slab.h>
> #include <linux/types.h>
>
> #include <linux/pinctrl/pinconf-generic.h>
> #include <linux/pinctrl/pinconf.h>
> #include <linux/pinctrl/pinctrl.h>
> #include <linux/pinctrl/pinmux.h>
>
> #include "core.h"
> #include "pinctrl-utils.h"
Thanks, looks much better to me!
[...]
> > > + if (WARN_ON(offset > 31))
> > > + return false;
> >
> > When this condition can be true?
>
> If there is a bug in the code. Defensive programming.
>
> There is this subtle conversion of pin numbers => offset inside of a
> bank. If one function forgets doing this then eq5p_test_bit() gets
> called with a pin number.
>
> In this GPIO series I fixed such a bug in a 10 year old driver:
> https://lore.kernel.org/lkml/[email protected]/
>
> The whole "if it can happen it will happen" mantra. We'll get a warning
> in the logs using pinctrl-eyeq5.
My point here that we have mechanisms to avoid such issues, for example in GPIO
we have valid_mask field and GPIO library takes care to avoid such conditions
from happening. Please, double check that you really need these in your driver.
I prefer to avoid them until it's proven that they are real cases.
..
> > > +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,
> >
> > ifdef is missing for these... But the question is, isn't these a default when
> > OF is in use?
>
> Doesn't look like it is. In drivers/pinctrl/devicetree.c:
>
> static int dt_to_map_one_config(struct pinctrl *p,
> struct pinctrl_dev *hog_pctldev,
> const char *statename,
> struct device_node *np_config)
> {
> // ...
>
> /*
> * Call pinctrl driver to parse device tree node, and
> * generate mapping table entries
> */
> ops = pctldev->desc->pctlops;
> if (!ops->dt_node_to_map) {
> dev_err(p->dev, "pctldev %s doesn't support DT\n",
> dev_name(pctldev->dev));
> return -ENODEV;
> }
>
> // ...
> }
>
> And I see nowhere that puts a value if ->dt_node_to_map is empty.
>
> For dt_free_map, it is an optional value. If the field is NULL nothing
> is done. See dt_free_map() in the same file.
If we drop OF dependency, these fields might not be present in the structure
(by definition). Compilation won't succeed. Am I mistaken?
[...]
> > > + mask = BIT(offset);
> > > + val = is_gpio ? 0 : U32_MAX;
> >
> > I think you meant something else (semantically) than U32_MAX.
> > Perhaps GENMASK(31, 0)?
>
> To me the semantic of U32_MAX is the same.
Not at all. When we speak hardware we speak bits, bitfields, etc. We almost
never speaks software types and their limits (u32 is a pure software concept).
> I see where you are coming
> from. A better alternative however would be:
>
> mask = BIT(offset);
> val = is_gpio ? 0 : mask;
>
> That way the desire is clear and the code is simpler.
Yes, please follow the latter, much better than integer limits.
..
> > > + case PIN_CONFIG_DRIVE_STRENGTH:
> > > + offset *= 2; /* two bits per pin */
> > > + if (offset >= 32) {
> > > + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_HIGH]);
> > > + offset -= 32;
> > > + } else {
> > > + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_LOW]);
> > > + }
> >
> > I'm wondering why you can't use your helpers before multiplication?
>
> I'm unsure what helpers you are talking about?
Which give you the MMIO addresses.
> If the question is about why multiply before if-condition: I feel like
> multiplying first allows having the if condition be "offset >= 32".
> That explicits why we readl HIGH vs LOW regs.
[...]
> > > + if (arg > 3) {
> >
> > Magic number.
>
> Would 0b11 explicit why? The value is two bits wide, so 0 thru 3.
No, the
#define FOO_SELF_EXPLAING GENMASK(1, 0) // or 3 or 0b11
will.
..
> > Similar comments as per previous function.
>
> So GENMASK(1, 0) rather than 0b11. Or GENMASK(offset+1, offset).
Definitely not the latter one, it will generate awful code.
> Something else?
if it was another comment, I don't see in the context here...
--
With Best Regards,
Andy Shevchenko
Hello,
On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:
>
> ...
>
> > > > + u32 offset = id & GENMASK(7, 0);
> > > > + u32 domain = id >> 8;
> > >
> > > Perhaps
> > >
> > > u32 offset = (id & GENMASK(7, 0)) >> 0;
> > > u32 domain = (id & GENMASK(31, 8)) >> 8;
> > >
> > > for better understanding the split?
> >
> > Do the additional zero-bit-shift and GENMASK() help understanding? My
> > brain needs time to parse them to then notice they do nothing and
> > simplify the code in my head, back to the original version.
>
> In my opinion yes, as you exactly showing the split.
> But. The better is to use FIELD_GET().
I'll go with the FIELD_GET() option!
[...]
>
> > > > + priv->rcdev.of_node = np;
> > >
> > > It's better to use device_set_node().
> >
> > I don't see how device_set_node() can help? It works on struct device
> > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > no users of device_set_node() in drivers/reset/.
>
> No users doesn't mean it's good. The API is relatively "new" and takes
> care of two things:
> 1) it uses agnostic interface;
> 2) it doesn't require any firmware node direct dereference.
>
> The 2) is most important here as allows us to refactor (firmware node) code
> in the future.
I think I get the point of device_set_node(). I still do not understand
how it could help me fill the ->of_node field in a reset_controller_dev
structure?
Should I be using device_set_node() to fill the struct device pointer
and the reset subsystem, by some magic, will pick this up and use it
for its own of_node field? I've not seen any magic/code doing that.
[...]
> > > > + priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
> > >
> > > Please, use corresponding hweightXX() API.
> >
> > Noted. I did not find this keyword even though I searched quite a bit
> > for it. "popcount" sounds more logical to me. :-)
>
> Hmm... But it's fundamental, it's called Hamming weight.
> https://en.wikipedia.org/wiki/Hamming_weight
Makes sense now. I've always called it population count following the
name of the matching instruction on x86 (and I believe other ISAs). TIL.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Thu, Feb 29, 2024 at 01:18:08PM +0100, Th?o Lebrun wrote:
> On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Th?o Lebrun wrote:
> > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Th?o Lebrun wrote:
..
> > > > > + priv->rcdev.of_node = np;
> > > >
> > > > It's better to use device_set_node().
> > >
> > > I don't see how device_set_node() can help? It works on struct device
> > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > no users of device_set_node() in drivers/reset/.
> >
> > No users doesn't mean it's good. The API is relatively "new" and takes
> > care of two things:
> > 1) it uses agnostic interface;
> > 2) it doesn't require any firmware node direct dereference.
> >
> > The 2) is most important here as allows us to refactor (firmware node) code
> > in the future.
>
> I think I get the point of device_set_node(). I still do not understand
> how it could help me fill the ->of_node field in a reset_controller_dev
> structure?
Exactly why I put the above comment as recommendation. And then I elaborated
that entire reset framework should rather move towards fwnode.
> Should I be using device_set_node() to fill the struct device pointer
> and the reset subsystem, by some magic, will pick this up and use it
> for its own of_node field? I've not seen any magic/code doing that.
At bare minimum it will give beneficial things:
1) less burden in the drivers conversion in case fwnode happens (and I believe
it's just matter of time) in reset framework;
2) hiding fwnode/of_node implemetation details (which is currently is layering
violation to some extend (as we have a lot of *of_*() APIs to avoid direct
access to of_node field in struct device).
The downside is that you will need to include property.h for this only thing.
And I don't see other code that can be converted to fwnode right away here.
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 29, 2024 at 03:48:59PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 01:18:08PM +0100, Th?o Lebrun wrote:
..
> The downside is that you will need to include property.h for this only thing.
> And I don't see other code that can be converted to fwnode right away here.
I meant here
device_set_node(..., dev_fwnode(parent));
On the second thought it can survive probably without it in a form
device_set_node(..., of_fwnode_handle(parent->of_node));
but this does not fully solve the fundamental problem with accessing of_node.
--
With Best Regards,
Andy Shevchenko
Hello,
On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote:
> On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
[...]
> > > > + u32 reg; /* next 8 bytes are r0 and r1 */
> > >
> > > Not sure this comments gives any clarification to a mere reader of the code.
> > > Perhaps you want to name this as reg64 (at least it will show that you have
> > > 8 bytes, but I have no clue what is the semantic relationship between r0 and
> > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> >
> > Clocks are defined by two 32-bit registers. We only store the first
> > register offset because they always follow each other.
>
> > I like the reg64 name and will remove the comment. This straight forward
> > code is found in the rest of the code, I don't think it is anything
> > hard to understand (ie does not need a comment):
> >
> > u32 r0 = readl(base_plls + pll->reg);
> > u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
>
> Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h)
> can be used in this case? It will be much better overall and be aligned with
> reg64 name.
The doc talks in terms of 32-bit registers. I do not see a reason to
work in 64-bit. If we get a 64-bit value that we need to split we need
to think about the endianness of our platform, which makes things more
complex than just reading both values independently.
> [...]
>
> > > I didn't get. If eq5c_init() was finished successfully, why do you need to
> > > seems repeat what it already done? What did I miss?
> >
> > The key here is that eq5c_init() iterates on eq5c_early_plls[] while
> > eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
> > commit message:
> >
> > > 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.
> >
> > Doing everything in eq5c_init() is not clean because we expect all new
> > clock provider drivers to be standard platform drivers. Doing
> > everything from a platform driver probe doesn't work because some
> > clocks are required earlier than platform bus init. We therefore do a
> > mix.
>
> Am I missing something or these two pieces are using the same IO resources?
> This looks like a lot of code duplication without clear benefit. Perhaps
> you can have a helper?
There are two subtle differences that make creating a helper difficult:
- Logging, pr_*() vs dev_*(). Second option is preferred but only
available once a device is created.
- Behavior on error: we stop the world for early clocks but keep going
for normal clocks.
[...]
> > > > + eq5c_clk_data->hws[pll->index] = hw;
> > > > + if (IS_ERR(hw))
> > >
> > > > + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> > > > + pll->name);
> > >
> > > Missed return statement?
> >
> > No, we still try to register all clocks even if one failed. I guess we
> > can call this being optimistic.
>
> But how critical these clocks are? I believe we should panic it we have no
> critical calls be available. Otherwise, why '_err_'? Shouldn't be dev_warn()?
Indeed printing should be dev_warn(), I missed that.
Thanks Andy,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Thu, Feb 29, 2024 at 03:27:01PM +0100, Th?o Lebrun wrote:
> On Wed, Feb 28, 2024 at 03:33:29PM +0100, Th?o Lebrun wrote:
> > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Th?o Lebrun wrote:
[...]
> > > > > + u32 reg; /* next 8 bytes are r0 and r1 */
> > > >
> > > > Not sure this comments gives any clarification to a mere reader of the code.
> > > > Perhaps you want to name this as reg64 (at least it will show that you have
> > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and
> > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> > >
> > > Clocks are defined by two 32-bit registers. We only store the first
> > > register offset because they always follow each other.
> >
> > > I like the reg64 name and will remove the comment. This straight forward
> > > code is found in the rest of the code, I don't think it is anything
> > > hard to understand (ie does not need a comment):
> > >
> > > u32 r0 = readl(base_plls + pll->reg);
> > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
> >
> > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h)
> > can be used in this case? It will be much better overall and be aligned with
> > reg64 name.
>
> The doc talks in terms of 32-bit registers. I do not see a reason to
> work in 64-bit. If we get a 64-bit value that we need to split we need
> to think about the endianness of our platform, which makes things more
> complex than just reading both values independently.
1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO.
2) Still I see a benefit from using lo_hi_readq() and friends directly.
[...]
> > > > I didn't get. If eq5c_init() was finished successfully, why do you need to
> > > > seems repeat what it already done? What did I miss?
> > >
> > > The key here is that eq5c_init() iterates on eq5c_early_plls[] while
> > > eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
> > > commit message:
> > >
> > > > 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.
> > >
> > > Doing everything in eq5c_init() is not clean because we expect all new
> > > clock provider drivers to be standard platform drivers. Doing
> > > everything from a platform driver probe doesn't work because some
> > > clocks are required earlier than platform bus init. We therefore do a
> > > mix.
> >
> > Am I missing something or these two pieces are using the same IO resources?
> > This looks like a lot of code duplication without clear benefit. Perhaps
> > you can have a helper?
>
> There are two subtle differences that make creating a helper difficult:
>
> - Logging, pr_*() vs dev_*(). Second option is preferred but only
> available once a device is created.
Some code uses (yeah, arguable that it's better, but depends on how much
the real deduplication takes)
if (dev)
dev_*(...);
else
pr_*(...);
> - Behavior on error: we stop the world for early clocks but keep going
> for normal clocks.
..(..., bool skip_errors)
{
...
}
(with the same caveat)?
--
With Best Regards,
Andy Shevchenko
Hello,
On Thu Feb 29, 2024 at 12:35 PM CET, Andy Shevchenko wrote:
> On Wed, Feb 28, 2024 at 07:15:12PM +0100, Théo Lebrun wrote:
> > On Tue Feb 27, 2024 at 7:14 PM CET, Andy Shevchenko wrote:
> > > On Tue, Feb 27, 2024 at 03:55:26PM +0100, Théo Lebrun wrote:
>
> ...
>
> > > > + bool "Mobileye EyeQ5 pinctrl driver"
> > >
> > > Can't be a module?
> >
> > It theory it could, I however do not see why that would be done. Pinctrl
> > is essential to the platform capabilities. The platform is an embedded
> > one and performance-oriented; boot-time is important and no user will
> > ever want to load pinctrl as a module.
>
> I can argue. The modularization can give a better granularity in the exactly
> embedded world when the memory resource (flash/RAM) is limited or fragmented
> (for one or another reason). Having less weighty kernel at boot makes it smaller
> to fit, for example, faster read only memory block which is not so uncommon.
I can argue back. :-) Granularity brought from modules is useful either
in (1) resource constrained boot context or (2) for peripherals which
some people might want to do without. We are not in case 1 nor case 2.
> The rule of thumb is to make modules if, otherwise, it's not so critical for
> the boot process (and even for some cases we still may have it done as a module
> with help of deferred probe mechanism).
I'd call SoC pin control a critical resource for the boot process.
I also like the simplicity of builtin better for such a resource.
- If we tristate pinctrl-eyeq5 and there is a bug, there is a bug (in a
context that we have no reason to support).
- If we do not allow it and there is a bug, there is no bug.
Plus, it makes one less choice for people configuring the kernel.
[...]
> > > > + if (WARN_ON(offset > 31))
> > > > + return false;
> > >
> > > When this condition can be true?
> >
> > If there is a bug in the code. Defensive programming.
> >
> > There is this subtle conversion of pin numbers => offset inside of a
> > bank. If one function forgets doing this then eq5p_test_bit() gets
> > called with a pin number.
> >
> > In this GPIO series I fixed such a bug in a 10 year old driver:
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > The whole "if it can happen it will happen" mantra. We'll get a warning
> > in the logs using pinctrl-eyeq5.
>
> My point here that we have mechanisms to avoid such issues, for example in GPIO
> we have valid_mask field and GPIO library takes care to avoid such conditions
> from happening. Please, double check that you really need these in your driver.
> I prefer to avoid them until it's proven that they are real cases.
Whatever the subsystem does to protect us (like only calling callbacks
with valid IDs), it will not protect us from bugs inside the driver's
callbacks.
I do no see a reason to avoid such code. I do not trust myself to write
perfect code. Its aim is to protect ourselves from our own mistakes. If
such an issue occurs, understanding that this is what happened would be
really hard (especially if it occurs on someone else's boards).
> ...
>
> > > > +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,
> > >
> > > ifdef is missing for these... But the question is, isn't these a default when
> > > OF is in use?
> >
> > Doesn't look like it is. In drivers/pinctrl/devicetree.c:
> >
> > static int dt_to_map_one_config(struct pinctrl *p,
> > struct pinctrl_dev *hog_pctldev,
> > const char *statename,
> > struct device_node *np_config)
> > {
> > // ...
> >
> > /*
> > * Call pinctrl driver to parse device tree node, and
> > * generate mapping table entries
> > */
> > ops = pctldev->desc->pctlops;
> > if (!ops->dt_node_to_map) {
> > dev_err(p->dev, "pctldev %s doesn't support DT\n",
> > dev_name(pctldev->dev));
> > return -ENODEV;
> > }
> >
> > // ...
> > }
> >
> > And I see nowhere that puts a value if ->dt_node_to_map is empty.
> >
> > For dt_free_map, it is an optional value. If the field is NULL nothing
> > is done. See dt_free_map() in the same file.
>
> If we drop OF dependency, these fields might not be present in the structure
> (by definition). Compilation won't succeed. Am I mistaken?
struct pinctrl_ops has both ->dt_node_to_map and ->dt_free_map fields in
any case. See include/linux/pinctrl/pinctrl.h which declares the
struct. The function pointers we put are also under no conditional
compilation.
[...]
> > > > + case PIN_CONFIG_DRIVE_STRENGTH:
> > > > + offset *= 2; /* two bits per pin */
> > > > + if (offset >= 32) {
> > > > + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_HIGH]);
> > > > + offset -= 32;
> > > > + } else {
> > > > + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_LOW]);
> > > > + }
> > >
> > > I'm wondering why you can't use your helpers before multiplication?
> >
> > I'm unsure what helpers you are talking about?
>
> Which give you the MMIO addresses.
Again sorry, but I don't get the question. I see no helper function that
returns an MMIO address in eq5p_pinconf_get(). Two helpers exist to
deal with memory accesses: eq5p_test_bit() and eq5p_update_bits().
Neither are called in this function nor could they be used.
> > If the question is about why multiply before if-condition: I feel like
> > multiplying first allows having the if condition be "offset >= 32".
> > That explicits why we readl HIGH vs LOW regs.
>
> [...]
>
> > > > + if (arg > 3) {
> > >
> > > Magic number.
> >
> > Would 0b11 explicit why? The value is two bits wide, so 0 thru 3.
>
> No, the
>
> #define FOO_SELF_EXPLAING GENMASK(1, 0) // or 3 or 0b11
>
> will.
Will do!
Thanks Andy,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello,
On Thu Feb 29, 2024 at 2:48 PM CET, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 01:18:08PM +0100, Théo Lebrun wrote:
> > On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:
>
> ...
>
> > > > > > + priv->rcdev.of_node = np;
> > > > >
> > > > > It's better to use device_set_node().
> > > >
> > > > I don't see how device_set_node() can help? It works on struct device
> > > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > > no users of device_set_node() in drivers/reset/.
> > >
> > > No users doesn't mean it's good. The API is relatively "new" and takes
> > > care of two things:
> > > 1) it uses agnostic interface;
> > > 2) it doesn't require any firmware node direct dereference.
> > >
> > > The 2) is most important here as allows us to refactor (firmware node) code
> > > in the future.
> >
> > I think I get the point of device_set_node(). I still do not understand
> > how it could help me fill the ->of_node field in a reset_controller_dev
> > structure?
>
> Exactly why I put the above comment as recommendation. And then I elaborated
> that entire reset framework should rather move towards fwnode.
OK now I get it. One question: would using fwnode abstractions make
sense for a driver that is devicetree-only, and will stay that way?
However this sounds out-of-scope of such a driver addition. I also am
not familiar enough (yet?) with the reset subsystem and/or fwnode to be
able to bring this kind of changes upstream.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Do, 2024-02-29 at 15:48 +0200, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 01:18:08PM +0100, Théo Lebrun wrote:
> > On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:
>
> ...
>
> > > > > > + priv->rcdev.of_node = np;
> > > > >
> > > > > It's better to use device_set_node().
> > > >
> > > > I don't see how device_set_node() can help? It works on struct device
> > > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > > no users of device_set_node() in drivers/reset/.
> > >
> > > No users doesn't mean it's good. The API is relatively "new" and takes
> > > care of two things:
> > > 1) it uses agnostic interface;
> > > 2) it doesn't require any firmware node direct dereference.
> > >
> > > The 2) is most important here as allows us to refactor (firmware node) code
> > > in the future.
> >
> > I think I get the point of device_set_node(). I still do not understand
> > how it could help me fill the ->of_node field in a reset_controller_dev
> > structure?
>
> Exactly why I put the above comment as recommendation. And then I elaborated
> that entire reset framework should rather move towards fwnode.
For context, there have been initial patches for this, that turned out
not to be necessary later on:
https://lore.kernel.org/lkml/[email protected]/
At this point, there still is no real use case for non-DT reset
controls on the horizon.
>
regards
Philipp
On Thu, Feb 29, 2024 at 04:23:01PM +0100, Th?o Lebrun wrote:
> On Thu Feb 29, 2024 at 2:48 PM CET, Andy Shevchenko wrote:
> > On Thu, Feb 29, 2024 at 01:18:08PM +0100, Th?o Lebrun wrote:
> > > On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > > > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Th?o Lebrun wrote:
> > > > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Th?o Lebrun wrote:
..
> > > > > > > + priv->rcdev.of_node = np;
> > > > > >
> > > > > > It's better to use device_set_node().
> > > > >
> > > > > I don't see how device_set_node() can help? It works on struct device
> > > > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > > > no users of device_set_node() in drivers/reset/.
> > > >
> > > > No users doesn't mean it's good. The API is relatively "new" and takes
> > > > care of two things:
> > > > 1) it uses agnostic interface;
> > > > 2) it doesn't require any firmware node direct dereference.
> > > >
> > > > The 2) is most important here as allows us to refactor (firmware node) code
> > > > in the future.
> > >
> > > I think I get the point of device_set_node(). I still do not understand
> > > how it could help me fill the ->of_node field in a reset_controller_dev
> > > structure?
> >
> > Exactly why I put the above comment as recommendation. And then I elaborated
> > that entire reset framework should rather move towards fwnode.
>
> OK now I get it. One question: would using fwnode abstractions make
> sense for a driver that is devicetree-only, and will stay that way?
In my opinion, yes. But less beneficial from it.
> However this sounds out-of-scope of such a driver addition. I also am
> not familiar enough (yet?) with the reset subsystem and/or fwnode to be
> able to bring this kind of changes upstream.
Right.
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 29, 2024 at 04:28:42PM +0100, Philipp Zabel wrote:
> On Do, 2024-02-29 at 15:48 +0200, Andy Shevchenko wrote:
> > On Thu, Feb 29, 2024 at 01:18:08PM +0100, Th?o Lebrun wrote:
> > > On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > > > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Th?o Lebrun wrote:
> > > > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Th?o Lebrun wrote:
..
> > > > > > > + priv->rcdev.of_node = np;
> > > > > >
> > > > > > It's better to use device_set_node().
> > > > >
> > > > > I don't see how device_set_node() can help? It works on struct device
> > > > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > > > no users of device_set_node() in drivers/reset/.
> > > >
> > > > No users doesn't mean it's good. The API is relatively "new" and takes
> > > > care of two things:
> > > > 1) it uses agnostic interface;
> > > > 2) it doesn't require any firmware node direct dereference.
> > > >
> > > > The 2) is most important here as allows us to refactor (firmware node) code
> > > > in the future.
> > >
> > > I think I get the point of device_set_node(). I still do not understand
> > > how it could help me fill the ->of_node field in a reset_controller_dev
> > > structure?
> >
> > Exactly why I put the above comment as recommendation. And then I elaborated
> > that entire reset framework should rather move towards fwnode.
>
> For context, there have been initial patches for this, that turned out
> not to be necessary later on:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> At this point, there still is no real use case for non-DT reset
> controls on the horizon.
I can argue on that if we have something like reset-gpio (and we have a such).
With this in place the ACPI can also provide descriptions for that.
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 29, 2024 at 04:13:15PM +0100, Th?o Lebrun wrote:
> On Thu Feb 29, 2024 at 12:35 PM CET, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2024 at 07:15:12PM +0100, Th?o Lebrun wrote:
> > > On Tue Feb 27, 2024 at 7:14 PM CET, Andy Shevchenko wrote:
> > > > On Tue, Feb 27, 2024 at 03:55:26PM +0100, Th?o Lebrun wrote:
..
> > > > > + bool "Mobileye EyeQ5 pinctrl driver"
> > > >
> > > > Can't be a module?
> > >
> > > It theory it could, I however do not see why that would be done. Pinctrl
> > > is essential to the platform capabilities. The platform is an embedded
> > > one and performance-oriented; boot-time is important and no user will
> > > ever want to load pinctrl as a module.
> >
> > I can argue. The modularization can give a better granularity in the exactly
> > embedded world when the memory resource (flash/RAM) is limited or fragmented
> > (for one or another reason). Having less weighty kernel at boot makes it smaller
> > to fit, for example, faster read only memory block which is not so uncommon.
>
> I can argue back. :-) Granularity brought from modules is useful either
> in (1) resource constrained boot context or (2) for peripherals which
> some people might want to do without. We are not in case 1 nor case 2.
>
> > The rule of thumb is to make modules if, otherwise, it's not so critical for
> > the boot process (and even for some cases we still may have it done as a module
> > with help of deferred probe mechanism).
>
> I'd call SoC pin control a critical resource for the boot process.
>
> I also like the simplicity of builtin better for such a resource.
> - If we tristate pinctrl-eyeq5 and there is a bug, there is a bug (in a
> context that we have no reason to support).
> - If we do not allow it and there is a bug, there is no bug.
> Plus, it makes one less choice for people configuring the kernel.
The problem is that you reduce the flexibility. Nobody prevents you from having
it built-in while tristate. But completely different situation when it's bool.
So my argument still stays. I think new code shouldn't be boolean by default.
The only exceptional cases can do that (like PMIC driver or critical clock one).
[...]
> > > > > + if (WARN_ON(offset > 31))
> > > > > + return false;
> > > >
> > > > When this condition can be true?
> > >
> > > If there is a bug in the code. Defensive programming.
> > >
> > > There is this subtle conversion of pin numbers => offset inside of a
> > > bank. If one function forgets doing this then eq5p_test_bit() gets
> > > called with a pin number.
> > >
> > > In this GPIO series I fixed such a bug in a 10 year old driver:
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > The whole "if it can happen it will happen" mantra. We'll get a warning
> > > in the logs using pinctrl-eyeq5.
> >
> > My point here that we have mechanisms to avoid such issues, for example in GPIO
> > we have valid_mask field and GPIO library takes care to avoid such conditions
> > from happening. Please, double check that you really need these in your driver.
> > I prefer to avoid them until it's proven that they are real cases.
>
> Whatever the subsystem does to protect us (like only calling callbacks
> with valid IDs), it will not protect us from bugs inside the driver's
> callbacks.
>
> I do no see a reason to avoid such code. I do not trust myself to write
> perfect code.
Perfect is enemy of good. ;)
> Its aim is to protect ourselves from our own mistakes. If
> such an issue occurs, understanding that this is what happened would be
> really hard (especially if it occurs on someone else's boards).
Yes, but we usually don't put a dead code into the kernel. So, can you confirm
that warning can appear IRL? If yes, there is another red flag or question:
why WARN()? This is easily becomes a panic and/or reboot (depending to the kernel
command line) and hence may give unresponsive system. Was this considered?
..
> > > > > +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,
> > > >
> > > > ifdef is missing for these... But the question is, isn't these a default when
> > > > OF is in use?
> > >
> > > Doesn't look like it is. In drivers/pinctrl/devicetree.c:
> > >
> > > static int dt_to_map_one_config(struct pinctrl *p,
> > > struct pinctrl_dev *hog_pctldev,
> > > const char *statename,
> > > struct device_node *np_config)
> > > {
> > > // ...
> > >
> > > /*
> > > * Call pinctrl driver to parse device tree node, and
> > > * generate mapping table entries
> > > */
> > > ops = pctldev->desc->pctlops;
> > > if (!ops->dt_node_to_map) {
> > > dev_err(p->dev, "pctldev %s doesn't support DT\n",
> > > dev_name(pctldev->dev));
> > > return -ENODEV;
> > > }
> > >
> > > // ...
> > > }
> > >
> > > And I see nowhere that puts a value if ->dt_node_to_map is empty.
> > >
> > > For dt_free_map, it is an optional value. If the field is NULL nothing
> > > is done. See dt_free_map() in the same file.
> >
> > If we drop OF dependency, these fields might not be present in the structure
> > (by definition). Compilation won't succeed. Am I mistaken?
>
> struct pinctrl_ops has both ->dt_node_to_map and ->dt_free_map fields in
> any case. See include/linux/pinctrl/pinctrl.h which declares the
> struct. The function pointers we put are also under no conditional
> compilation.
Indeed, I mixed it with something else (probably GPIO library and one of its
core structures) where it's the case.
--
With Best Regards,
Andy Shevchenko
Hello,
On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote:
> > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote:
> > > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
>
> [...]
>
> > > > > > + u32 reg; /* next 8 bytes are r0 and r1 */
> > > > >
> > > > > Not sure this comments gives any clarification to a mere reader of the code.
> > > > > Perhaps you want to name this as reg64 (at least it will show that you have
> > > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and
> > > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> > > >
> > > > Clocks are defined by two 32-bit registers. We only store the first
> > > > register offset because they always follow each other.
> > >
> > > > I like the reg64 name and will remove the comment. This straight forward
> > > > code is found in the rest of the code, I don't think it is anything
> > > > hard to understand (ie does not need a comment):
> > > >
> > > > u32 r0 = readl(base_plls + pll->reg);
> > > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
> > >
> > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h)
> > > can be used in this case? It will be much better overall and be aligned with
> > > reg64 name.
> >
> > The doc talks in terms of 32-bit registers. I do not see a reason to
> > work in 64-bit. If we get a 64-bit value that we need to split we need
> > to think about the endianness of our platform, which makes things more
> > complex than just reading both values independently.
>
> 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO.
Just tested, it works. No error on the memory bus. And checked assembly
generated was a single 64-bit instructions.
It might not work on other hardware revisions though. I can't remember
if memory bus is changing across them.
> 2) Still I see a benefit from using lo_hi_readq() and friends directly.
So it is:
u32 r0 = readl(base_plls + pll->reg64);
u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0));
vs:
u64 r = lo_hi_readq(base_plls + pll->regs64);
u32 r0 = r;
u32 r1 = r >> 32;
One is straight forward, the other uses an obscure helper that code
readers must understand and follows that with bit manipulation.
>
> [...]
>
> > > > > I didn't get. If eq5c_init() was finished successfully, why do you need to
> > > > > seems repeat what it already done? What did I miss?
> > > >
> > > > The key here is that eq5c_init() iterates on eq5c_early_plls[] while
> > > > eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
> > > > commit message:
> > > >
> > > > > 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.
> > > >
> > > > Doing everything in eq5c_init() is not clean because we expect all new
> > > > clock provider drivers to be standard platform drivers. Doing
> > > > everything from a platform driver probe doesn't work because some
> > > > clocks are required earlier than platform bus init. We therefore do a
> > > > mix.
> > >
> > > Am I missing something or these two pieces are using the same IO resources?
> > > This looks like a lot of code duplication without clear benefit. Perhaps
> > > you can have a helper?
> >
> > There are two subtle differences that make creating a helper difficult:
> >
> > - Logging, pr_*() vs dev_*(). Second option is preferred but only
> > available once a device is created.
>
> Some code uses (yeah, arguable that it's better, but depends on how much
> the real deduplication takes)
>
> if (dev)
> dev_*(...);
> else
> pr_*(...);
>
> > - Behavior on error: we stop the world for early clocks but keep going
> > for normal clocks.
>
> ...(..., bool skip_errors)
> {
> ...
> }
>
> (with the same caveat)?
I started trying it out, but the combination of both flags means dealing
with errors would look like:
ret = foo();
if (ret) {
if (!skip_errors) {
if (dev)
dev_err(dev, "...");
else
pr_err("...");
return ret;
}
if (dev)
dev_warn(dev, "...");
else
pr_warn("...");
}
There are two errors to handle, that makes a mess out of the code.
Having a little bit of repetition but straight forward code is nicer in
my opinion. At least we tried!
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Thu, Feb 29, 2024 at 04:40:25PM +0100, Th?o Lebrun wrote:
> On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote:
> > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Th?o Lebrun wrote:
> > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Th?o Lebrun wrote:
> > > > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > > > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Th?o Lebrun wrote:
[...]
> > > > > > > + u32 reg; /* next 8 bytes are r0 and r1 */
> > > > > >
> > > > > > Not sure this comments gives any clarification to a mere reader of the code.
> > > > > > Perhaps you want to name this as reg64 (at least it will show that you have
> > > > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and
> > > > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> > > > >
> > > > > Clocks are defined by two 32-bit registers. We only store the first
> > > > > register offset because they always follow each other.
> > > >
> > > > > I like the reg64 name and will remove the comment. This straight forward
> > > > > code is found in the rest of the code, I don't think it is anything
> > > > > hard to understand (ie does not need a comment):
> > > > >
> > > > > u32 r0 = readl(base_plls + pll->reg);
> > > > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
> > > >
> > > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h)
> > > > can be used in this case? It will be much better overall and be aligned with
> > > > reg64 name.
> > >
> > > The doc talks in terms of 32-bit registers. I do not see a reason to
> > > work in 64-bit. If we get a 64-bit value that we need to split we need
> > > to think about the endianness of our platform, which makes things more
> > > complex than just reading both values independently.
> >
> > 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO.
>
> Just tested, it works. No error on the memory bus. And checked assembly
> generated was a single 64-bit instructions.
>
> It might not work on other hardware revisions though. I can't remember
> if memory bus is changing across them.
>
> > 2) Still I see a benefit from using lo_hi_readq() and friends directly.
>
> So it is:
>
> u32 r0 = readl(base_plls + pll->reg64);
> u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0));
>
> vs:
>
> u64 r = lo_hi_readq(base_plls + pll->regs64);
> u32 r0 = r;
> u32 r1 = r >> 32;
It depends to the semantics of these two. How hard do they coupled to each
other semantically? I.o.w. can they always be considered as 64-bit register
with the respective bitfields? (And note FIELD_GET() here is your friend.)
> One is straight forward, the other uses an obscure helper that code
> readers must understand and follows that with bit manipulation.
[...]
> There are two errors to handle, that makes a mess out of the code.
> Having a little bit of repetition but straight forward code is nicer in
> my opinion. At least we tried!
Yes! Perhaps you can add a couple of words into commit message to explain
this detail of implementation (that code in two parts is not so identical
to be easily deduplicated).
--
With Best Regards,
Andy Shevchenko
Hello,
On Thu Feb 29, 2024 at 4:48 PM CET, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 04:40:25PM +0100, Théo Lebrun wrote:
> > On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote:
> > > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote:
> > > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote:
> > > > > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > > > > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
>
> [...]
>
> > > > > > > > + u32 reg; /* next 8 bytes are r0 and r1 */
> > > > > > >
> > > > > > > Not sure this comments gives any clarification to a mere reader of the code.
> > > > > > > Perhaps you want to name this as reg64 (at least it will show that you have
> > > > > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and
> > > > > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> > > > > >
> > > > > > Clocks are defined by two 32-bit registers. We only store the first
> > > > > > register offset because they always follow each other.
> > > > >
> > > > > > I like the reg64 name and will remove the comment. This straight forward
> > > > > > code is found in the rest of the code, I don't think it is anything
> > > > > > hard to understand (ie does not need a comment):
> > > > > >
> > > > > > u32 r0 = readl(base_plls + pll->reg);
> > > > > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
> > > > >
> > > > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h)
> > > > > can be used in this case? It will be much better overall and be aligned with
> > > > > reg64 name.
> > > >
> > > > The doc talks in terms of 32-bit registers. I do not see a reason to
> > > > work in 64-bit. If we get a 64-bit value that we need to split we need
> > > > to think about the endianness of our platform, which makes things more
> > > > complex than just reading both values independently.
> > >
> > > 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO.
> >
> > Just tested, it works. No error on the memory bus. And checked assembly
> > generated was a single 64-bit instructions.
> >
> > It might not work on other hardware revisions though. I can't remember
> > if memory bus is changing across them.
> >
> > > 2) Still I see a benefit from using lo_hi_readq() and friends directly.
> >
> > So it is:
> >
> > u32 r0 = readl(base_plls + pll->reg64);
> > u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0));
> >
> > vs:
> >
> > u64 r = lo_hi_readq(base_plls + pll->regs64);
>
> > u32 r0 = r;
> > u32 r1 = r >> 32;
>
> It depends to the semantics of these two. How hard do they coupled to each
> other semantically? I.o.w. can they always be considered as 64-bit register
> with the respective bitfields? (And note FIELD_GET() here is your friend.)
OLB (the memory region) has always been described as a list of 32-bit
registers. The semantics lean in the camp of two readl().
> > One is straight forward, the other uses an obscure helper that code
> > readers must understand and follows that with bit manipulation.
>
> [...]
>
> > There are two errors to handle, that makes a mess out of the code.
> > Having a little bit of repetition but straight forward code is nicer in
> > my opinion. At least we tried!
>
> Yes! Perhaps you can add a couple of words into commit message to explain
> this detail of implementation (that code in two parts is not so identical
> to be easily deduplicated).
Yes, will do. I get why from a reader's point-of-view it looks like
duplicate code.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi folks,
lots of discussion here, lazy Reviewed-by from me, but Andy often (thank God!)
catches things I just miss.
On Thu, Feb 29, 2024 at 4:32 PM Andy Shevchenko
<[email protected]> wrote:
> > > The rule of thumb is to make modules if, otherwise, it's not so critical for
> > > the boot process (and even for some cases we still may have it done as a module
> > > with help of deferred probe mechanism).
> >
> > I'd call SoC pin control a critical resource for the boot process.
> >
> > I also like the simplicity of builtin better for such a resource.
> > - If we tristate pinctrl-eyeq5 and there is a bug, there is a bug (in a
> > context that we have no reason to support).
> > - If we do not allow it and there is a bug, there is no bug.
> > Plus, it makes one less choice for people configuring the kernel.
>
> The problem is that you reduce the flexibility. Nobody prevents you from having
> it built-in while tristate. But completely different situation when it's bool.
>
> So my argument still stays. I think new code shouldn't be boolean by default.
> The only exceptional cases can do that (like PMIC driver or critical clock one).
I think bool is helpful for users if:
- The system cannot boot without the pin control driver
- The system cannot mount root from a storage medium without the pin control
driver. Initramfs doesn't count for embedded systems, many of these use things
like OpenWrt that does not use initramfs the way Debian or Fedora etc does.
This SoC is obviously for the deeply embedded usecase. If this SoC has root
on flash or eMMC and cannot access either without pin control, it is helpful
for users to have this as bool so they don't shoot themselves in the foot with
Kconfig.
> > > > > > + if (WARN_ON(offset > 31))
> > > > > > + return false;
I think I asked for this code in my review, because I felt unsafe about offset.
Maybe it's not such a big problem, but, this twoliner is also not a big deal.
Yours,
Linus Walleij
Quoting Théo Lebrun (2024-02-29 07:40:25)
> Hello,
>
> On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote:
> > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote:
> > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote:
> > > > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > > > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
>
> > 2) Still I see a benefit from using lo_hi_readq() and friends directly.
>
> So it is:
>
> u32 r0 = readl(base_plls + pll->reg64);
> u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0));
>
> vs:
>
> u64 r = lo_hi_readq(base_plls + pll->regs64);
> u32 r0 = r;
> u32 r1 = r >> 32;
>
> One is straight forward, the other uses an obscure helper that code
> readers must understand and follows that with bit manipulation.
>
Just use readq() and include the correct header please. We know what
readq() is in the kernel.
Hello,
On Tue Feb 27, 2024 at 5:38 PM CET, Rob Herring wrote:
> On Tue, 27 Feb 2024 15:55:23 +0100, Théo Lebrun wrote:
> > Add documentation to describe the "Other Logic Block" syscon.
> >
> > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > Signed-off-by: Théo Lebrun <[email protected]>
> > ---
> > .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 94 ++++++++++++++++++++++
> > 1 file changed, 94 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@2c: False schema does not allow {'compatible': ['mobileye,eyeq5-clk'], 'reg': [[44, 80], [284, 4]], 'reg-names': ['plls', 'ospi'], '#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@0: False schema does not allow {'compatible': ['mobileye,eyeq5-reset'], 'reg': [[0, 12], [512, 52], [288, 4]], 'reg-names': ['d0', 'd1', 'd2'], '#reset-cells': [[2]]}
> from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
> Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.exampledtb: /example-0/soc/system-controller@e00000/reset-controller@0: failed to match any schema with compatible: ['mobileye,eyeq5-reset']
> Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.exampledtb: /example-0/soc/system-controller@e00000/clock-controller@2c: failed to match any schema with compatible: ['mobileye,eyeq5-clk']
This series depends on 4 patches from previous revisions that got taken
into clk-next. Those are v6.8-rc1..clk-mobileye on the clk remote [0].
Without the 4 patches I've reproduced the above warning; it disappears
once they are applied.
Have a nice day,
[0]: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/log/?h=clk-mobileye
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Do, 2024-02-29 at 17:36 +0200, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 04:28:42PM +0100, Philipp Zabel wrote:
> > On Do, 2024-02-29 at 15:48 +0200, Andy Shevchenko wrote:
> > > [...] And then I elaborated that entire reset framework should
> > > rather move towards fwnode.
> >
> > For context, there have been initial patches for this, that turned out
> > not to be necessary later on:
> >
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > At this point, there still is no real use case for non-DT reset
> > controls on the horizon.
>
> I can argue on that if we have something like reset-gpio (and we have a such).
I've just sent out the pull request containing this, thank you for the
reminder.
> With this in place the ACPI can also provide descriptions for that.
Yes, an ACPI based device with shared GPIO resets (it is bound to
happen at some point...) would provide a reason to support ACPI GPIOs
in the reset framework.
regards
Philipp