| Message ID | 20240123-mbly-clk-v3-0-392b010b8281@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for Mobileye EyeQ5 system controller | expand |
On 23/01/2024 19:46, Théo Lebrun wrote: > Hi, > You miss here, in this place, the most important information which I asked previously - dependencies/merging: https://lore.kernel.org/all/db9b2786-29f7-4d45-9087-a9f85b770b6c@linaro.org/ > The goal of this series is to add clk, reset and pinctrl support for the > Mobileye EyeQ5 platform [0]. Control of those is grouped inside a > system controller block called "OLB". Best regards, Krzysztof
On 23/01/2024 19:46, Théo Lebrun wrote: > Add the Mobileye EyeQ5 reset controller driver. It belongs to a syscon > region called OLB. It might grow to add later support of other > platforms from Mobileye. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > MAINTAINERS | 1 + > drivers/reset/Kconfig | 12 ++ > drivers/reset/Makefile | 1 + > drivers/reset/reset-eyeq5.c | 383 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 397 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3ea96ab7d2b8..dd3b5834386f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14794,6 +14794,7 @@ F: arch/mips/boot/dts/mobileye/ > F: arch/mips/configs/eyeq5_defconfig > F: arch/mips/mobileye/board-epm5.its.S > F: drivers/clk/clk-eyeq5.c > +F: drivers/reset/reset-eyeq5.c > F: include/dt-bindings/clock/mobileye,eyeq5-clk.h > F: include/dt-bindings/soc/mobileye,eyeq5.h > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index ccd59ddd7610..80bfde54c076 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -66,6 +66,18 @@ config RESET_BRCMSTB_RESCAL > This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on > BCM7216. > > +config RESET_EYEQ5 > + bool "Mobileye EyeQ5 reset controller" > + depends on MFD_SYSCON > + depends on MACH_EYEQ5 || COMPILE_TEST > + default MACH_EYEQ5 > + help > + This enables the Mobileye EyeQ5 reset controller. > + > + It has three domains, with a varying number of resets in each of them. > + Registers are located in a shared register region called OLB accessed > + through a syscon & regmap. > + > config RESET_HSDK > bool "Synopsys HSDK Reset Driver" > depends on HAS_IOMEM > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 8270da8a4baa..4fabe0070390 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o > obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o > obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o > obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o > +obj-$(CONFIG_RESET_EYEQ5) += reset-eyeq5.o > obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o > obj-$(CONFIG_RESET_IMX7) += reset-imx7.o > obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o > diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c > new file mode 100644 > index 000000000000..2217e42e140b > --- /dev/null > +++ b/drivers/reset/reset-eyeq5.c > @@ -0,0 +1,383 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Reset driver for the Mobileye EyeQ5 platform. > + * > + * The registers are located in a syscon region called OLB. We handle three > + * reset domains. Domains 0 and 2 look similar in that they both use one bit > + * per reset line. Domain 1 has a register per reset. > + * > + * We busy-wait after updating a reset in domains 0 or 1. The reason is hardware > + * logic built-in self-test (LBIST) that might be enabled. > + * > + * We use eq5r_ as prefix, as-in "EyeQ5 Reset", but way shorter. > + * > + * Known resets in domain 0: > + * 3. CAN0 > + * 4. CAN1 > + * 5. CAN2 > + * 6. SPI0 > + * 7. SPI1 > + * 8. SPI2 > + * 9. SPI3 > + * 10. UART0 > + * 11. UART1 > + * 12. UART2 > + * 13. I2C0 > + * 14. I2C1 > + * 15. I2C2 > + * 16. I2C3 > + * 17. I2C4 > + * 18. TIMER0 > + * 19. TIMER1 > + * 20. TIMER2 > + * 21. TIMER3 > + * 22. TIMER4 > + * 23. WD0 > + * 24. EXT0 > + * 25. EXT1 > + * 26. GPIO > + * 27. WD1 > + * > + * Known resets in domain 1: > + * 0. VMP0 (Vector Microcode Processors) > + * 1. VMP1 > + * 2. VMP2 > + * 3. VMP3 > + * 4. PMA0 (Programmable Macro Array) > + * 5. PMA1 > + * 6. PMAC0 > + * 7. PMAC1 > + * 8. MPC0 (Multi-threaded Processing Clusters) > + * 9. MPC1 > + * > + * Known resets in domain 2: > + * 0. PCIE0_CORE > + * 1. PCIE0_APB > + * 2. PCIE0_LINK_AXI > + * 3. PCIE0_LINK_MGMT > + * 4. PCIE0_LINK_HOT > + * 5. PCIE0_LINK_PIPE > + * 6. PCIE1_CORE > + * 7. PCIE1_APB > + * 8. PCIE1_LINK_AXI > + * 9. PCIE1_LINK_MGMT > + * 10. PCIE1_LINK_HOT > + * 11. PCIE1_LINK_PIPE > + * 12. MULTIPHY > + * 13. MULTIPHY_APB > + * 15. PCIE0_LINK_MGMT > + * 16. PCIE1_LINK_MGMT > + * 17. PCIE0_LINK_PM > + * 18. PCIE1_LINK_PM > + * > + * Copyright (C) 2024 Mobileye Vision Technologies Ltd. > + */ > + > +#include <linux/mfd/syscon.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/reset-controller.h> > + > +/* Offsets into the OLB region as well as masks for domain 1 registers. */ > +#define EQ5R_OLB_SARCR0 (0x004) > +#define EQ5R_OLB_SARCR1 (0x008) > +#define EQ5R_OLB_PCIE_GP (0x120) > +#define EQ5R_OLB_ACRP_REG(n) (0x200 + 4 * (n)) // n=0..12 > +#define EQ5R_OLB_ACRP_PD_REQ BIT(0) > +#define EQ5R_OLB_ACRP_ST_POWER_DOWN BIT(27) > +#define EQ5R_OLB_ACRP_ST_ACTIVE BIT(29) > + > +/* Vendor-provided values. D1 has a long timeout because of LBIST. */ > +#define D0_TIMEOUT_POLL 10 > +#define D1_TIMEOUT_POLL 40000 > + > +/* > + * Masks for valid reset lines in each domain. This array is also used to get > + * the domain and reset counts. > + */ > +static const u32 eq5r_valid_masks[] = { 0x0FFFFFF8, 0x00001FFF, 0x0007BFFF }; > + > +#define EQ5R_DOMAIN_COUNT ARRAY_SIZE(eq5r_valid_masks) > + > +struct eq5r_private { > + struct mutex mutexes[EQ5R_DOMAIN_COUNT]; /* We serialize all reset operations. */ > + struct regmap *olb; /* Writes go to a syscon regmap. */ > + struct reset_controller_dev rcdev; > +}; > + > +static int _eq5r_busy_wait(struct eq5r_private *priv, struct device *dev, > + u32 domain, u32 offset, bool assert) > +{ > + unsigned int val, mask; > + int i; > + > + lockdep_assert_held(&priv->mutexes[domain]); > + > + switch (domain) { > + case 0: > + for (i = 0; i < D0_TIMEOUT_POLL; i++) { > + regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val); > + val = !(val & BIT(offset)); > + if (val == assert) > + return 0; > + __udelay(1); What is even "__udelay"? It is the first use in drivers. Please use common methods, like fsleep or udelay... but actually you should rather use regmap_read_poll_timeout() or some variants instead of open-coding it. > + } > + break; > + case 1: > + mask = assert ? EQ5R_OLB_ACRP_ST_POWER_DOWN : EQ5R_OLB_ACRP_ST_ACTIVE; > + for (i = 0; i < D1_TIMEOUT_POLL; i++) { > + regmap_read(priv->olb, EQ5R_OLB_ACRP_REG(offset), &val); > + if (val & mask) > + return 0; > + __udelay(1); > + } > + break; > + case 2: > + return 0; /* No busy waiting for domain 2. */ > + default: > + WARN_ON(1); > + return -EINVAL; > + } > + > + dev_dbg(dev, "%u-%u: timeout\n", domain, offset); > + return -ETIMEDOUT; > +} > + > +static void _eq5r_assert(struct eq5r_private *priv, u32 domain, u32 offset) Drop leading _ and name the function in some informative way. > +{ > + lockdep_assert_held(&priv->mutexes[domain]); > + > + switch (domain) { > + case 0: > + regmap_clear_bits(priv->olb, EQ5R_OLB_SARCR0, BIT(offset)); > + break; > + case 1: > + regmap_set_bits(priv->olb, EQ5R_OLB_ACRP_REG(offset), > + EQ5R_OLB_ACRP_PD_REQ); > + break; > + case 2: > + regmap_clear_bits(priv->olb, EQ5R_OLB_PCIE_GP, BIT(offset)); > + break; > + default: > + WARN_ON(1); > + } > +} > + > +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct eq5r_private *priv = dev_get_drvdata(rcdev->dev); > + u32 offset = id & GENMASK(7, 0); > + u32 domain = id >> 8; > + int ret; > + > + if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT)) > + return -EINVAL; > + > + dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset); > + > + mutex_lock(&priv->mutexes[domain]); > + _eq5r_assert(priv, domain, offset); > + ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, true); > + mutex_unlock(&priv->mutexes[domain]); > + > + return ret; > +} > + > +static void _eq5r_deassert(struct eq5r_private *priv, u32 domain, u32 offset) > +{ > + lockdep_assert_held(&priv->mutexes[domain]); > + > + switch (domain) { > + case 0: > + regmap_set_bits(priv->olb, EQ5R_OLB_SARCR0, BIT(offset)); > + break; > + case 1: > + regmap_clear_bits(priv->olb, EQ5R_OLB_ACRP_REG(offset), > + EQ5R_OLB_ACRP_PD_REQ); > + break; > + case 2: > + regmap_set_bits(priv->olb, EQ5R_OLB_PCIE_GP, BIT(offset)); > + break; > + default: > + WARN_ON(1); > + } > +} > + > +static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct eq5r_private *priv = dev_get_drvdata(rcdev->dev); > + u32 offset = id & GENMASK(7, 0); > + u32 domain = id >> 8; > + int ret; > + > + if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT)) > + return -EINVAL; > + > + dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset); > + > + mutex_lock(&priv->mutexes[domain]); > + _eq5r_deassert(priv, domain, offset); > + ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, false); > + mutex_unlock(&priv->mutexes[domain]); > + > + return ret; > +} > + > +static int eq5r_reset(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct device *dev = rcdev->dev; > + struct eq5r_private *priv = dev_get_drvdata(dev); > + u32 offset = id & GENMASK(7, 0); > + u32 domain = id >> 8; > + int ret; > + > + if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT)) > + return -EINVAL; > + > + dev_dbg(dev, "%u-%u: reset request\n", domain, offset); > + > + mutex_lock(&priv->mutexes[domain]); > + > + _eq5r_assert(priv, domain, offset); > + ret = _eq5r_busy_wait(priv, dev, domain, offset, true); > + if (ret) /* don't let an error disappear silently */ > + dev_warn(dev, "%u-%u: reset assert failed: %d\n", > + domain, offset, ret); > + > + _eq5r_deassert(priv, domain, offset); > + ret = _eq5r_busy_wait(priv, dev, domain, offset, false); > + > + mutex_unlock(&priv->mutexes[domain]); > + > + return ret; > +} > + > +static int eq5r_status(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct eq5r_private *priv = dev_get_drvdata(rcdev->dev); > + u32 offset = id & GENMASK(7, 0); > + u32 domain = id >> 8; > + unsigned int val; > + int ret; > + > + if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT)) > + return -EINVAL; > + > + dev_dbg(rcdev->dev, "%u-%u: status request\n", domain, offset); > + > + mutex_lock(&priv->mutexes[domain]); > + > + switch (domain) { > + case 0: > + regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val); > + ret = !(val & BIT(offset)); > + break; > + case 1: > + regmap_read(priv->olb, EQ5R_OLB_ACRP_REG(offset), &val); > + ret = !(val & EQ5R_OLB_ACRP_ST_ACTIVE); > + break; > + case 2: > + regmap_read(priv->olb, EQ5R_OLB_PCIE_GP, &val); > + ret = !(val & BIT(offset)); > + break; > + } > + > + mutex_unlock(&priv->mutexes[domain]); > + > + return ret; > +} > + > +static const struct reset_control_ops eq5r_ops = { > + .reset = eq5r_reset, > + .assert = eq5r_assert, > + .deassert = eq5r_deassert, > + .status = eq5r_status, > +}; > + > +static int eq5r_of_xlate(struct reset_controller_dev *rcdev, > + const struct of_phandle_args *reset_spec) > +{ > + u32 domain, offset; > + > + if (WARN_ON(reset_spec->args_count != 2)) > + return -EINVAL; > + > + domain = reset_spec->args[0]; > + offset = reset_spec->args[1]; > + > + if (domain >= EQ5R_DOMAIN_COUNT || offset > 31 || > + !(eq5r_valid_masks[domain] & BIT(offset))) { > + dev_err(rcdev->dev, "%u-%u: invalid reset\n", domain, offset); > + return -EINVAL; > + } > + > + return (domain << 8) | offset; > +} > + > +static int eq5r_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct device_node *parent_np = of_get_parent(np); > + struct eq5r_private *priv; > + int ret, i; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; You leak parent. > + > + dev_set_drvdata(dev, priv); > + > + priv->olb = ERR_PTR(-ENODEV); > + if (parent_np) { > + priv->olb = syscon_node_to_regmap(parent_np); > + of_node_put(parent_np); > + } > + if (IS_ERR(priv->olb)) Also here > + return PTR_ERR(priv->olb); This looks over-complicated. First, you cannot just dev_get_regmap(pdev->dev.parent)? > + > + for (i = 0; i < EQ5R_DOMAIN_COUNT; i++) > + mutex_init(&priv->mutexes[i]); > + > + priv->rcdev.ops = &eq5r_ops; > + priv->rcdev.owner = THIS_MODULE; > + priv->rcdev.dev = dev; > + priv->rcdev.of_node = np; > + priv->rcdev.of_reset_n_cells = 2; > + priv->rcdev.of_xlate = eq5r_of_xlate; > + > + priv->rcdev.nr_resets = 0; > + for (i = 0; i < EQ5R_DOMAIN_COUNT; i++) > + priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]); > + > + ret = reset_controller_register(&priv->rcdev); > + if (ret) { > + dev_err(dev, "Failed registering reset controller: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static const struct of_device_id eq5r_match_table[] = { > + { .compatible = "mobileye,eyeq5-reset" }, > + {} > +}; > + > +static struct platform_driver eq5r_driver = { > + .probe = eq5r_probe, > + .driver = { > + .name = "eyeq5-reset", > + .of_match_table = eq5r_match_table, > + }, > +}; > + > +static int __init eq5r_init(void) > +{ > + return platform_driver_register(&eq5r_driver); > +} > + > +arch_initcall(eq5r_init); This is does not look like arch code, but driver or subsys. Use regular module_driver. I see there is such pattern in reset but I doubt this is something good. Best regards, Krzysztof
On 23/01/2024 19:46, Théo Lebrun wrote: > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later > support of other platforms from Mobileye. It belongs to a syscon region > called OLB. > > Existing pins and their function live statically in the driver code > rather than in the devicetree, see compatible match data. > ... > +static int eq5p_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct device_node *parent_np = of_get_parent(np); > + const struct eq5p_match *match = of_device_get_match_data(dev); > + struct pinctrl_dev *pctldev; > + struct eq5p_pinctrl *pctrl; > + int ret; > + > + pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL); > + if (!pctrl) > + return -ENOMEM; > + > + pctrl->olb = ERR_PTR(-ENODEV); > + if (parent_np) > + pctrl->olb = syscon_node_to_regmap(parent_np); > + if (IS_ERR(pctrl->olb)) > + pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb"); > + if (IS_ERR(pctrl->olb)) > + return PTR_ERR(pctrl->olb); No, we talked about this, you got comments on this. There is no mobileye,olb. You cannot have undocumented properties. > + > + pctrl->regs = match->regs; > + pctrl->funcs = match->funcs; > + pctrl->nfuncs = match->nfuncs; > + > + pctrl->desc.name = dev_name(dev); > + pctrl->desc.pins = match->pins; > + pctrl->desc.npins = match->npins; > + pctrl->desc.pctlops = &eq5p_pinctrl_ops; > + pctrl->desc.pmxops = &eq5p_pinmux_ops; > + pctrl->desc.confops = &eq5p_pinconf_ops; > + pctrl->desc.owner = THIS_MODULE; > + > + ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev); > + if (ret) { > + dev_err(dev, "Failed registering pinctrl device: %d\n", ret); > + return ret; > + } > + > + ret = pinctrl_enable(pctldev); > + if (ret) { > + dev_err(dev, "Failed enabling pinctrl device: %d\n", ret); > + return ret; > + } > + > + dev_info(dev, "probed\n"); I am pretty sure you got comments for these. Drop such debugs from all of your code. Current and future. > + > + return 0; > +} > + > +static const struct eq5p_match eq5p_match_a = { > + .regs = { > + [EQ5P_PD] = 0x0C0, > + [EQ5P_PU] = 0x0C4, > + [EQ5P_DS_LOW] = 0x0D0, > + [EQ5P_DS_HIGH] = 0x0D4, > + [EQ5P_IOCR] = 0x0B0, > + }, > + .pins = eq5p_pins_a, > + .npins = ARRAY_SIZE(eq5p_pins_a), > + .funcs = eq5p_functions_a, > + .nfuncs = ARRAY_SIZE(eq5p_functions_a), > +}; > + > +static const struct eq5p_match eq5p_match_b = { > + .regs = { > + [EQ5P_PD] = 0x0C8, > + [EQ5P_PU] = 0x0CC, > + [EQ5P_DS_LOW] = 0x0D8, > + [EQ5P_DS_HIGH] = 0x0DC, > + [EQ5P_IOCR] = 0x0B4, > + }, > + .pins = eq5p_pins_b, > + .npins = ARRAY_SIZE(eq5p_pins_b), > + .funcs = eq5p_functions_b, > + .nfuncs = ARRAY_SIZE(eq5p_functions_b), > +}; > + > +static const struct of_device_id eq5p_match[] = { > + { .compatible = "mobileye,eyeq5-a-pinctrl", .data = &eq5p_match_a }, > + { .compatible = "mobileye,eyeq5-b-pinctrl", .data = &eq5p_match_b }, > + {}, > +}; > + > +static struct platform_driver eq5p_driver = { > + .driver = { > + .name = "eyeq5-pinctrl", > + .of_match_table = eq5p_match, > + }, > + .probe = eq5p_probe, > +}; > + > +static int __init eq5p_init(void) > +{ > + return platform_driver_register(&eq5p_driver); > +} > +core_initcall(eq5p_init); No, pins are not a core_initcall. This could be arch_initcall, but considering you depend on the parent this must be module driver. Even from this dependency point of view your initcalls are totally wrong and will lead to issues. Best regards, Krzysztof
On 23/01/2024 19:46, Théo Lebrun wrote: > Add the Mobileye EyeQ5 clock controller driver. It might grow to add > support for other platforms from Mobileye. > > It handles 10 read-only PLLs derived from the main crystal on board. It > exposes a table-based divider clock used for OSPI. Other platform > clocks are not configurable and therefore kept as fixed-factor > devicetree nodes. > > Two PLLs are required early on and are therefore registered at > of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the > UARTs. > > +#define OLB_PCSR1_RESET BIT(0) > +#define OLB_PCSR1_SSGC_DIV GENMASK(4, 1) > +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */ > +#define OLB_PCSR1_SPREAD GENMASK(9, 5) > +#define OLB_PCSR1_DIS_SSCG BIT(10) > +/* Down-spread or center-spread */ > +#define OLB_PCSR1_DOWN_SPREAD BIT(11) > +#define OLB_PCSR1_FRAC_IN GENMASK(31, 12) > + > +static struct clk_hw_onecell_data *eq5c_clk_data; > +static struct regmap *eq5c_olb; Drop these two. No file-scope regmaps for drivers. Use private container structures. ... > +static void __init eq5c_init(struct device_node *np) > +{ > + struct device_node *parent_np = of_get_parent(np); > + int i, ret; > + > + eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS), > + GFP_KERNEL); > + if (!eq5c_clk_data) { > + ret = -ENOMEM; > + goto err; > + } > + > + eq5c_clk_data->num = EQ5C_NB_CLKS; > + > + /* > + * Mark all clocks as deferred. We register some now and others at > + * platform device probe. > + */ > + for (i = 0; i < EQ5C_NB_CLKS; i++) > + eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER); > + > + /* > + * Currently, if OLB is not available, we log an error, fail init then How it could be not available? Only with broken initcall ordering. Fix your initcall ordering and then simplify all this weird code. > + * fail probe. We might want to change this behavior and assume all > + * clocks are in bypass mode; this is what is being done in the vendor > + * driver. > + * > + * It is unclear if there are valid situations where the OLB region > + * would be inaccessible. Best regards, Krzysztof
On 1/23/24 9:46 PM, Théo Lebrun wrote: > Remove the `reg-io-width` property from the olb@e00000 syscon. The The diff says it's system-controller@e00000 now... :-) > default memory access width is what we desire: no need to make it > explicit. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > arch/mips/boot/dts/mobileye/eyeq5.dtsi | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi > index 81497febcdee..03e7e942ee22 100644 > --- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi > +++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi > @@ -103,7 +103,6 @@ uart2: serial@a00000 { > olb: system-controller@e00000 { > compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; > reg = <0 0xe00000 0x0 0x400>; > - reg-io-width = <4>; > }; > > gic: interrupt-controller@140000 { MBR, Sergey
On Di, 2024-01-23 at 19:46 +0100, Théo Lebrun wrote: [...] > diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c > new file mode 100644 > index 000000000000..2217e42e140b > --- /dev/null > +++ b/drivers/reset/reset-eyeq5.c > @@ -0,0 +1,383 @@ [...] > +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct eq5r_private *priv = dev_get_drvdata(rcdev->dev); rcdev is contained in priv, you can just use container_of instead of chasing pointers around. > + u32 offset = id & GENMASK(7, 0); > + u32 domain = id >> 8; > + int ret; > + > + if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT)) > + return -EINVAL; Reset controls with domain >= EQ5R_DOMAIN_COUNT are already weeded out during request by of_xlate, so this check is not necessary. > + dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset); > + > + mutex_lock(&priv->mutexes[domain]); > + _eq5r_assert(priv, domain, offset); > + ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, true); > + mutex_unlock(&priv->mutexes[domain]); > + > + return ret; Consider using guard(mutex)(&priv->mutexes[domain]) from linux/cleanup.h to automatically unlock on return. [...] > +static int eq5r_reset(struct reset_controller_dev *rcdev, unsigned long id) Is this used by anything? If unused, I'd prefer this not to be implemented. If it is used, is no delay required between assert and deassert by any consumer? > +{ > + struct device *dev = rcdev->dev; > + struct eq5r_private *priv = dev_get_drvdata(dev); > + u32 offset = id & GENMASK(7, 0); > + u32 domain = id >> 8; > + int ret; > + > + if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT)) > + return -EINVAL; > + > + dev_dbg(dev, "%u-%u: reset request\n", domain, offset); > + > + mutex_lock(&priv->mutexes[domain]); > + > + _eq5r_assert(priv, domain, offset); > + ret = _eq5r_busy_wait(priv, dev, domain, offset, true); > + if (ret) /* don't let an error disappear silently */ > + dev_warn(dev, "%u-%u: reset assert failed: %d\n", > + domain, offset, ret); Why not return the error though? > + _eq5r_deassert(priv, domain, offset); > + ret = _eq5r_busy_wait(priv, dev, domain, offset, false); > + > + mutex_unlock(&priv->mutexes[domain]); > + > + return ret; > +} [...] > +static int eq5r_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct device_node *parent_np = of_get_parent(np); > + struct eq5r_private *priv; > + int ret, i; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); Using devm_kzalloc() avoids leaking this on error return or driver unbind. > + if (!priv) > + return -ENOMEM; > + > + dev_set_drvdata(dev, priv); > + > + priv->olb = ERR_PTR(-ENODEV); > + if (parent_np) { > + priv->olb = syscon_node_to_regmap(parent_np); > + of_node_put(parent_np); > + } > + if (IS_ERR(priv->olb)) > + return PTR_ERR(priv->olb); > + > + for (i = 0; i < EQ5R_DOMAIN_COUNT; i++) > + mutex_init(&priv->mutexes[i]); > + > + priv->rcdev.ops = &eq5r_ops; > + priv->rcdev.owner = THIS_MODULE; > + priv->rcdev.dev = dev; > + priv->rcdev.of_node = np; > + priv->rcdev.of_reset_n_cells = 2; > + priv->rcdev.of_xlate = eq5r_of_xlate; > + > + priv->rcdev.nr_resets = 0; > + for (i = 0; i < EQ5R_DOMAIN_COUNT; i++) > + priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]); > + > + ret = reset_controller_register(&priv->rcdev); Similarly, use devm_reset_controller_register() or disable driver unbind with suppress_bind_attrs. regards Philipp
On Tue, Jan 23, 2024 at 07:46:55PM +0100, Théo Lebrun wrote: > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later > support of other platforms from Mobileye. It belongs to a syscon region > called OLB. > > Existing pins and their function live statically in the driver code > rather than in the devicetree, see compatible match data. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > MAINTAINERS | 1 + > drivers/pinctrl/Kconfig | 15 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-eyeq5.c | 595 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 612 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index dd3b5834386f..9c423a4feb86 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14794,6 +14794,7 @@ F: arch/mips/boot/dts/mobileye/ > F: arch/mips/configs/eyeq5_defconfig > F: arch/mips/mobileye/board-epm5.its.S > F: drivers/clk/clk-eyeq5.c > +F: drivers/pinctrl/pinctrl-eyeq5.c > F: drivers/reset/reset-eyeq5.c > F: include/dt-bindings/clock/mobileye,eyeq5-clk.h > F: include/dt-bindings/soc/mobileye,eyeq5.h > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 8163a5983166..abe94de85b3d 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -195,6 +195,21 @@ config PINCTRL_EQUILIBRIUM > desired pin functions, configure GPIO attributes for LGM SoC pins. > Pin muxing and pin config settings are retrieved from device tree. > > +config PINCTRL_EYEQ5 > + bool "Mobileye EyeQ5 pinctrl driver" > + depends on OF > + depends on MACH_EYEQ5 || COMPILE_TEST > + select PINMUX > + select GENERIC_PINCONF > + select MFD_SYSCON > + default MACH_EYEQ5 > + help > + Pin controller driver for the Mobileye EyeQ5 platform. It does both > + pin config & pin muxing. It does not handle GPIO. > + > + Pin muxing supports two functions for each pin: first is GPIO, second > + is pin-dependent. Pin config is about bias & drive strength. > + > config PINCTRL_GEMINI > bool > depends on ARCH_GEMINI > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index 1071f301cc70..0033940914d9 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o > obj-$(CONFIG_PINCTRL_DA9062) += pinctrl-da9062.o > obj-$(CONFIG_PINCTRL_DIGICOLOR) += pinctrl-digicolor.o > obj-$(CONFIG_PINCTRL_EQUILIBRIUM) += pinctrl-equilibrium.o > +obj-$(CONFIG_PINCTRL_EYEQ5) += pinctrl-eyeq5.o > obj-$(CONFIG_PINCTRL_GEMINI) += pinctrl-gemini.o > obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o > obj-$(CONFIG_PINCTRL_K210) += pinctrl-k210.o > diff --git a/drivers/pinctrl/pinctrl-eyeq5.c b/drivers/pinctrl/pinctrl-eyeq5.c > new file mode 100644 > index 000000000000..2d8e5b2168bd > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-eyeq5.c > @@ -0,0 +1,595 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Pinctrl driver for the Mobileye EyeQ5 platform. > + * > + * The registers are located in a syscon region called OLB. There are two pin > + * banks, each being controlled by 5 registers (see enum eq5p_regs) for > + * pull-down, pull-up, drive strength and muxing. > + * > + * For each pin, muxing is between two functions: (0) GPIO or (1) another one > + * that is pin-dependent. Functions are declared statically in this driver. > + * > + * We create pinctrl groups that are 1:1 equivalent to pins: each group has a > + * single pin, and its index/selector is the pin number/offset. > + * > + * We use eq5p_ as prefix, as-in "EyeQ5 Pinctrl", but way shorter. > + * > + * Copyright (C) 2024 Mobileye Vision Technologies Ltd. > + */ > +#include <linux/mfd/syscon.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/pinctrl/pinconf-generic.h> > +#include <linux/pinctrl/pinconf.h> > +#include <linux/pinctrl/pinctrl.h> > +#include <linux/pinctrl/pinmux.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/seq_file.h> > + > +#include "core.h" > +#include "pinctrl-utils.h" > + > +#define GPIO_FUNC_SELECTOR 0 > + > +struct eq5p_pinctrl { > + struct pinctrl_desc desc; > + > + struct regmap *olb; > + const unsigned int *regs; /* array of size EQ5P_REG_MAX */ > + > + const struct eq5p_function *funcs; > + unsigned int nfuncs; > +}; > + > +struct eq5p_function { > + const char *name; > + const char * const *groups; > + unsigned int ngroups; > +}; > + > +/* OLB registers; those are offsets in an array of address offsets. */ > +enum eq5p_regs { > + EQ5P_PD, > + EQ5P_PU, > + EQ5P_DS_LOW, > + EQ5P_DS_HIGH, > + EQ5P_IOCR, > + > + EQ5P_REG_MAX > +}; > + > +static int eq5p_pinctrl_get_groups_count(struct pinctrl_dev *pctldev) > +{ > + return pctldev->desc->npins; > +} > + > +static const char *eq5p_pinctrl_get_group_name(struct pinctrl_dev *pctldev, > + unsigned int selector) > +{ > + return pctldev->desc->pins[selector].name; > +} > + > +static int eq5p_pinctrl_get_group_pins(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const unsigned int **pins, > + unsigned int *num_pins) > +{ > + *pins = &pctldev->desc->pins[selector].number; > + *num_pins = 1; > + return 0; > +} > + > +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int offset, > + unsigned long *config); > + > +static void eq5p_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev, > + struct seq_file *s, > + unsigned int offset) > +{ > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const char *pin_name = pctrl->desc.pins[offset].name; > + const char *func_name, *bias; > + unsigned int val_pd, val_pu, val_iocr; > + unsigned long ds_config; > + u32 drive_strength; > + bool pd, pu; > + int i, j; > + > + /* First, let's get the function name. */ > + regmap_read(pctrl->olb, pctrl->regs[EQ5P_IOCR], &val_iocr); > + if ((val_iocr & BIT(offset)) == 0) { > + func_name = pctrl->funcs[GPIO_FUNC_SELECTOR].name; > + } else { > + /* All pins have only two functions: GPIO and something else. We > + * look for this something else. > + */ > + func_name = NULL; > + for (i = 0; i < pctrl->nfuncs; i++) { > + if (i == GPIO_FUNC_SELECTOR) > + continue; > + > + for (j = 0; j < pctrl->funcs[i].ngroups; j++) { > + /* Groups and pins are the same thing for us. */ > + const char *x = pctrl->funcs[i].groups[j]; > + > + if (strcmp(x, pin_name) == 0) { > + func_name = pctrl->funcs[i].name; > + break; > + } > + } > + > + if (func_name) > + break; > + } > + > + /* We have not found the function attached to this pin, this > + * should never occur as all pins have exactly two functions. > + */ > + if (!func_name) > + func_name = "unknown"; > + } > + > + /* Second, we retrieve the bias. */ > + regmap_read(pctrl->olb, pctrl->regs[EQ5P_PD], &val_pd); > + pd = (val_pd & BIT(offset)) != 0; > + regmap_read(pctrl->olb, pctrl->regs[EQ5P_PU], &val_pu); > + pu = (val_pu & BIT(offset)) != 0; > + if (pd && pu) > + bias = "both"; > + else if (pd && !pu) > + bias = "pulldown"; > + else if (!pd && pu) > + bias = "pullup"; > + else > + bias = "none"; > + > + /* Third, we get the drive strength. */ > + ds_config = pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH, 0); > + eq5p_pinconf_get(pctldev, offset, &ds_config); > + drive_strength = pinconf_to_config_argument(ds_config); > + > + seq_printf(s, "function=%s bias=%s drive_strength=%d", > + func_name, bias, drive_strength); > +} > + > +static const struct pinctrl_ops eq5p_pinctrl_ops = { > + .get_groups_count = eq5p_pinctrl_get_groups_count, > + .get_group_name = eq5p_pinctrl_get_group_name, > + .get_group_pins = eq5p_pinctrl_get_group_pins, > + .pin_dbg_show = eq5p_pinctrl_pin_dbg_show, > + .dt_node_to_map = pinconf_generic_dt_node_to_map_pin, > + .dt_free_map = pinctrl_utils_free_map, > +}; > + > +static int eq5p_pinmux_get_functions_count(struct pinctrl_dev *pctldev) > +{ > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + > + return pctrl->nfuncs; > +} > + > +static const char *eq5p_pinmux_get_function_name(struct pinctrl_dev *pctldev, > + unsigned int selector) > +{ > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + > + return pctrl->funcs[selector].name; > +} > + > +static int eq5p_pinmux_get_function_groups(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const char * const **groups, > + unsigned int *num_groups) > +{ > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + > + *groups = pctrl->funcs[selector].groups; > + *num_groups = pctrl->funcs[selector].ngroups; > + return 0; > +} > + > +static int eq5p_pinmux_set_mux(struct pinctrl_dev *pctldev, > + unsigned int func_selector, unsigned int offset) > +{ > + const char *group_name = pctldev->desc->pins[offset].name; > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const char *func_name = pctrl->funcs[func_selector].name; > + bool is_gpio = func_selector == GPIO_FUNC_SELECTOR; > + unsigned int reg, mask, val; > + > + dev_dbg(pctldev->dev, "%s: func=%s group=%s\n", __func__, func_name, > + group_name); > + > + reg = pctrl->regs[EQ5P_IOCR]; > + mask = BIT(offset); > + val = is_gpio ? 0 : U32_MAX; > + > + regmap_update_bits(pctrl->olb, reg, mask, val); > + > + return 0; > +} > + > +static int eq5p_pinmux_gpio_request_enable(struct pinctrl_dev *pctldev, > + struct pinctrl_gpio_range *range, > + unsigned int offset) > +{ > + /* Pin offsets and group selectors are the same thing in our case. */ > + return eq5p_pinmux_set_mux(pctldev, GPIO_FUNC_SELECTOR, offset); > +} > + > +static const struct pinmux_ops eq5p_pinmux_ops = { > + .get_functions_count = eq5p_pinmux_get_functions_count, > + .get_function_name = eq5p_pinmux_get_function_name, > + .get_function_groups = eq5p_pinmux_get_function_groups, > + .set_mux = eq5p_pinmux_set_mux, > + .gpio_request_enable = eq5p_pinmux_gpio_request_enable, > + .strict = true, > +}; > + > +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int offset, > + unsigned long *config) > +{ > + enum pin_config_param param = pinconf_to_config_param(*config); > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + unsigned int reg, val_pd, val_pu, val_ds; > + bool pd, pu; > + u32 arg = 0; > + > + regmap_read(pctrl->olb, pctrl->regs[EQ5P_PD], &val_pd); > + pd = (val_pd & BIT(offset)) != 0; > + regmap_read(pctrl->olb, pctrl->regs[EQ5P_PU], &val_pu); > + pu = (val_pu & BIT(offset)) != 0; > + > + switch (param) { > + case PIN_CONFIG_BIAS_DISABLE: > + arg = !(pd || pu); > + break; > + case PIN_CONFIG_BIAS_PULL_DOWN: > + arg = pd; > + break; > + case PIN_CONFIG_BIAS_PULL_UP: > + arg = pu; > + break; > + case PIN_CONFIG_DRIVE_STRENGTH: > + offset *= 2; /* two bits per pin */ > + if (offset >= 32) { > + reg = pctrl->regs[EQ5P_DS_HIGH]; > + offset -= 32; > + } else { > + reg = pctrl->regs[EQ5P_DS_LOW]; > + } > + regmap_read(pctrl->olb, reg, &val_ds); > + arg = (val_ds >> offset) & 0b11; > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + *config = pinconf_to_config_packed(param, arg); > + return 0; > +} > + > +static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev, > + unsigned int offset, u32 arg) > +{ > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + unsigned int reg, mask, val; > + > + if (arg > 3) { > + dev_err(pctldev->dev, "Unsupported drive strength: %u\n", arg); > + return -EINVAL; > + } > + > + offset *= 2; /* two bits per pin */ > + > + if (offset >= 32) { > + reg = pctrl->regs[EQ5P_DS_HIGH]; > + offset -= 32; > + } else { > + reg = pctrl->regs[EQ5P_DS_LOW]; > + } > + > + mask = 0b11 << offset; > + val = arg << offset; > + regmap_update_bits(pctrl->olb, reg, mask, val); > + return 0; > +} > + > +static int eq5p_pinconf_set(struct pinctrl_dev *pctldev, unsigned int offset, > + unsigned long *configs, unsigned int num_configs) > +{ > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const char *pin_name = pctldev->desc->pins[offset].name; > + struct device *dev = pctldev->dev; > + unsigned int i, val, reg_pd, reg_pu; > + > + val = BIT(offset); > + reg_pd = pctrl->regs[EQ5P_PD]; > + reg_pu = pctrl->regs[EQ5P_PU]; > + > + for (i = 0; i < num_configs; i++) { > + enum pin_config_param param = pinconf_to_config_param(configs[i]); > + u32 arg = pinconf_to_config_argument(configs[i]); > + > + switch (param) { > + case PIN_CONFIG_BIAS_DISABLE: > + dev_dbg(dev, "%s: pin=%s BIAS_DISABLE\n", > + __func__, pin_name); > + regmap_clear_bits(pctrl->olb, reg_pd, val); > + regmap_clear_bits(pctrl->olb, reg_pu, val); > + break; > + case PIN_CONFIG_BIAS_PULL_DOWN: > + dev_dbg(dev, "%s: pin=%s BIAS_PULL_DOWN arg=%u\n", > + __func__, pin_name, arg); > + if (arg == 0) /* cannot connect to GND */ > + return -EOPNOTSUPP; > + regmap_set_bits(pctrl->olb, reg_pd, val); > + regmap_clear_bits(pctrl->olb, reg_pu, val); > + break; > + case PIN_CONFIG_BIAS_PULL_UP: > + dev_dbg(dev, "%s: pin=%s BIAS_PULL_UP arg=%u\n", > + __func__, pin_name, arg); > + if (arg == 0) /* cannot connect to VDD */ > + return -EOPNOTSUPP; > + regmap_clear_bits(pctrl->olb, reg_pd, val); > + regmap_set_bits(pctrl->olb, reg_pu, val); > + break; > + case PIN_CONFIG_DRIVE_STRENGTH: > + dev_dbg(dev, "%s: pin=%s DRIVE_STRENGTH arg=%u\n", > + __func__, pin_name, arg); > + eq5p_pinconf_set_drive_strength(pctldev, offset, arg); > + break; > + default: > + dev_err(dev, "Unsupported pinconf %u\n", param); > + return -EOPNOTSUPP; > + } > + } > + > + return 0; > +} > + > +static const struct pinconf_ops eq5p_pinconf_ops = { > + .is_generic = true, > + .pin_config_get = eq5p_pinconf_get, > + .pin_config_set = eq5p_pinconf_set, > + /* Pins and groups are equivalent in this driver. */ > + .pin_config_group_get = eq5p_pinconf_get, > + .pin_config_group_set = eq5p_pinconf_set, > +}; > + > +/* > + * Comments to the right of each pin are the "signal name" in the datasheet. > + */ > + > +static const struct pinctrl_pin_desc eq5p_pins_a[] = { /* Bank A */ > + PINCTRL_PIN(0, "PA0"), /* A0_TIMER0_CK */ > + PINCTRL_PIN(1, "PA1"), /* A1_TIMER0_EOC */ > + PINCTRL_PIN(2, "PA2"), /* A2_TIMER1_CK */ > + PINCTRL_PIN(3, "PA3"), /* A3_TIMER1_EOC */ > + PINCTRL_PIN(4, "PA4"), /* A4_TIMER2_CK */ > + PINCTRL_PIN(5, "PA5"), /* A5_TIMER2_EOC */ > + PINCTRL_PIN(6, "PA6"), /* A6_TIMER5_EXT_INCAP1 */ > + PINCTRL_PIN(7, "PA7"), /* A7_TIMER5_EXT_INCAP2 */ > + PINCTRL_PIN(8, "PA8"), /* A8_TIMER5_EXT_OUTCMP1 */ > + PINCTRL_PIN(9, "PA9"), /* A9_TIMER5_EXT_OUTCMP2 */ > + PINCTRL_PIN(10, "PA10"), /* A10_UART_0_TX */ > + PINCTRL_PIN(11, "PA11"), /* A11_UART_0_RX */ > + PINCTRL_PIN(12, "PA12"), /* A12_UART_1_TX */ > + PINCTRL_PIN(13, "PA13"), /* A13_UART_1_RX */ > + PINCTRL_PIN(14, "PA14"), /* A14_CAN_0_TX */ > + PINCTRL_PIN(15, "PA15"), /* A15_CAN_0_RX */ > + PINCTRL_PIN(16, "PA16"), /* A16_CAN_1_TX */ > + PINCTRL_PIN(17, "PA17"), /* A17_CAN_1_RX */ > + PINCTRL_PIN(18, "PA18"), /* A18_SPI_0_DO */ > + PINCTRL_PIN(19, "PA19"), /* A19_SPI_0_DI */ > + PINCTRL_PIN(20, "PA20"), /* A20_SPI_0_CK */ > + PINCTRL_PIN(21, "PA21"), /* A21_SPI_0_CS0 */ > + PINCTRL_PIN(22, "PA22"), /* A22_SPI_0_CS1 */ > + PINCTRL_PIN(23, "PA23"), /* A23_SPI_1_DO */ > + PINCTRL_PIN(24, "PA24"), /* A24_SPI_1_DI */ > + PINCTRL_PIN(25, "PA25"), /* A25_SPI_1_CK */ > + PINCTRL_PIN(26, "PA26"), /* A26_SPI_1_CS0 */ > + PINCTRL_PIN(27, "PA27"), /* A27_SPI_1_CS1 */ > + PINCTRL_PIN(28, "PA28"), /* A28_REF_CLK0 */ > +}; > + > +static const struct pinctrl_pin_desc eq5p_pins_b[] = { /* Bank B */ > + PINCTRL_PIN(0, "PB0"), /* B0_TIMER3_CK */ > + PINCTRL_PIN(1, "PB1"), /* B1_TIMER3_EOC */ > + PINCTRL_PIN(2, "PB2"), /* B2_TIMER4_CK */ > + PINCTRL_PIN(3, "PB3"), /* B3_TIMER4_EOC */ > + PINCTRL_PIN(4, "PB4"), /* B4_TIMER6_EXT_INCAP1 */ > + PINCTRL_PIN(5, "PB5"), /* B5_TIMER6_EXT_INCAP2 */ > + PINCTRL_PIN(6, "PB6"), /* B6_TIMER6_EXT_OUTCMP1 */ > + PINCTRL_PIN(7, "PB7"), /* B7_TIMER6_EXT_OUTCMP2 */ > + PINCTRL_PIN(8, "PB8"), /* B8_UART_2_TX */ > + PINCTRL_PIN(9, "PB9"), /* B9_UART_2_RX */ > + PINCTRL_PIN(10, "PB10"), /* B10_CAN_2_TX */ > + PINCTRL_PIN(11, "PB11"), /* B11_CAN_2_RX */ > + PINCTRL_PIN(12, "PB12"), /* B12_SPI_2_DO */ > + PINCTRL_PIN(13, "PB13"), /* B13_SPI_2_DI */ > + PINCTRL_PIN(14, "PB14"), /* B14_SPI_2_CK */ > + PINCTRL_PIN(15, "PB15"), /* B15_SPI_2_CS0 */ > + PINCTRL_PIN(16, "PB16"), /* B16_SPI_2_CS1 */ > + PINCTRL_PIN(17, "PB17"), /* B17_SPI_3_DO */ > + PINCTRL_PIN(18, "PB18"), /* B18_SPI_3_DI */ > + PINCTRL_PIN(19, "PB19"), /* B19_SPI_3_CK */ > + PINCTRL_PIN(20, "PB20"), /* B20_SPI_3_CS0 */ > + PINCTRL_PIN(21, "PB21"), /* B21_SPI_3_CS1 */ > + PINCTRL_PIN(22, "PB22"), /* B22_MCLK0 */ > +}; > + > +/* Groups of functions on bank A */ > +static const char * const gpioa_groups[] = { > + "PA0", "PA1", "PA2", "PA3", "PA4", "PA5", "PA6", "PA7", "PA8", "PA9", > + "PA10", "PA11", "PA12", "PA13", "PA14", "PA15", "PA16", "PA17", "PA18", > + "PA19", "PA20", "PA21", "PA22", "PA23", "PA24", "PA25", "PA26", "PA27", > + "PA28", > +}; > +static const char * const timer0_groups[] = { "PA0", "PA1" }; > +static const char * const timer1_groups[] = { "PA2", "PA3" }; > +static const char * const timer2_groups[] = { "PA4", "PA5" }; > +static const char * const timer5_groups[] = { "PA6", "PA7", "PA8", "PA9" }; > +static const char * const uart0_groups[] = { "PA10", "PA11" }; > +static const char * const uart1_groups[] = { "PA12", "PA13" }; > +static const char * const can0_groups[] = { "PA14", "PA15" }; > +static const char * const can1_groups[] = { "PA16", "PA17" }; > +static const char * const spi0_groups[] = { "PA18", "PA19", "PA20", "PA21", "PA22" }; > +static const char * const spi1_groups[] = { "PA23", "PA24", "PA25", "PA26", "PA27" }; > +static const char * const refclk0_groups[] = { "PA28" }; > + > +/* Groups of functions on bank B */ > +static const char * const gpiob_groups[] = { > + "PB0", "PB1", "PB2", "PB3", "PB4", "PB5", "PB6", "PB7", "PB8", "PB9", > + "PB10", "PB11", "PB12", "PB13", "PB14", "PB15", "PB16", "PB17", "PB18", > + "PB19", "PB20", "PB21", "PB22", > +}; > +static const char * const timer3_groups[] = { "PB0", "PB1" }; > +static const char * const timer4_groups[] = { "PB2", "PB3" }; > +static const char * const timer6_groups[] = { "PB4", "PB5", "PB6", "PB7" }; > +static const char * const uart2_groups[] = { "PB8", "PB9" }; > +static const char * const can2_groups[] = { "PB10", "PB11" }; > +static const char * const spi2_groups[] = { "PB12", "PB13", "PB14", "PB15", "PB16" }; > +static const char * const spi3_groups[] = { "PB17", "PB18", "PB19", "PB20", "PB21" }; > +static const char * const mclk0_groups[] = { "PB22" }; > + > +#define FUNCTION(a, b) { .name = a, .groups = b, .ngroups = ARRAY_SIZE(b) } > + > +static const struct eq5p_function eq5p_functions_a[] = { > + /* GPIO having a fixed index is depended upon, see GPIO_FUNC_SELECTOR. */ > + FUNCTION("gpio", gpioa_groups), > + > + FUNCTION("timer0", timer0_groups), > + FUNCTION("timer1", timer1_groups), > + FUNCTION("timer2", timer2_groups), > + FUNCTION("timer5", timer5_groups), > + FUNCTION("uart0", uart0_groups), > + FUNCTION("uart1", uart1_groups), > + FUNCTION("can0", can0_groups), > + FUNCTION("can1", can1_groups), > + FUNCTION("spi0", spi0_groups), > + FUNCTION("spi1", spi1_groups), > + FUNCTION("refclk0", refclk0_groups), > +}; > + > +static const struct eq5p_function eq5p_functions_b[] = { > + /* GPIO having a fixed index is depended upon, see GPIO_FUNC_SELECTOR. */ > + FUNCTION("gpio", gpiob_groups), > + > + FUNCTION("timer3", timer3_groups), > + FUNCTION("timer4", timer4_groups), > + FUNCTION("timer6", timer6_groups), > + FUNCTION("uart2", uart2_groups), > + FUNCTION("can2", can2_groups), > + FUNCTION("spi2", spi2_groups), > + FUNCTION("spi3", spi3_groups), > + FUNCTION("mclk0", mclk0_groups), > +}; > + > +struct eq5p_match { > + unsigned int regs[EQ5P_REG_MAX]; > + const struct pinctrl_pin_desc *pins; > + unsigned int npins; > + const struct eq5p_function *funcs; > + unsigned int nfuncs; > +}; > + > +static int eq5p_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct device_node *parent_np = of_get_parent(np); > + const struct eq5p_match *match = of_device_get_match_data(dev); > + struct pinctrl_dev *pctldev; > + struct eq5p_pinctrl *pctrl; > + int ret; > + > + pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL); > + if (!pctrl) > + return -ENOMEM; > + > + pctrl->olb = ERR_PTR(-ENODEV); > + if (parent_np) > + pctrl->olb = syscon_node_to_regmap(parent_np); > + if (IS_ERR(pctrl->olb)) > + pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb"); > + if (IS_ERR(pctrl->olb)) > + return PTR_ERR(pctrl->olb); > + > + pctrl->regs = match->regs; > + pctrl->funcs = match->funcs; > + pctrl->nfuncs = match->nfuncs; > + > + pctrl->desc.name = dev_name(dev); > + pctrl->desc.pins = match->pins; > + pctrl->desc.npins = match->npins; > + pctrl->desc.pctlops = &eq5p_pinctrl_ops; > + pctrl->desc.pmxops = &eq5p_pinmux_ops; > + pctrl->desc.confops = &eq5p_pinconf_ops; > + pctrl->desc.owner = THIS_MODULE; > + > + ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev); > + if (ret) { > + dev_err(dev, "Failed registering pinctrl device: %d\n", ret); > + return ret; > + } > + > + ret = pinctrl_enable(pctldev); > + if (ret) { > + dev_err(dev, "Failed enabling pinctrl device: %d\n", ret); > + return ret; > + } > + > + dev_info(dev, "probed\n"); > + > + return 0; > +} > + > +static const struct eq5p_match eq5p_match_a = { > + .regs = { > + [EQ5P_PD] = 0x0C0, > + [EQ5P_PU] = 0x0C4, > + [EQ5P_DS_LOW] = 0x0D0, > + [EQ5P_DS_HIGH] = 0x0D4, > + [EQ5P_IOCR] = 0x0B0, > + }, > + .pins = eq5p_pins_a, > + .npins = ARRAY_SIZE(eq5p_pins_a), > + .funcs = eq5p_functions_a, > + .nfuncs = ARRAY_SIZE(eq5p_functions_a), > +}; > + > +static const struct eq5p_match eq5p_match_b = { > + .regs = { > + [EQ5P_PD] = 0x0C8, > + [EQ5P_PU] = 0x0CC, > + [EQ5P_DS_LOW] = 0x0D8, > + [EQ5P_DS_HIGH] = 0x0DC, > + [EQ5P_IOCR] = 0x0B4, > + }, These are all the same relative offsets, so you really only need to store the base offset. The use of 2 compatibles is a bit questionable as the programming model appears to be the same and only which pins differ. Surely there are some other pinctrl drivers handling mutiple instances. Rob
Hello, On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote: > On 23/01/2024 19:46, Théo Lebrun wrote: > > Add the Mobileye EyeQ5 clock controller driver. It might grow to add > > support for other platforms from Mobileye. > > > > It handles 10 read-only PLLs derived from the main crystal on board. It > > exposes a table-based divider clock used for OSPI. Other platform > > clocks are not configurable and therefore kept as fixed-factor > > devicetree nodes. > > > > Two PLLs are required early on and are therefore registered at > > of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the > > UARTs. > > > > > > +#define OLB_PCSR1_RESET BIT(0) > > +#define OLB_PCSR1_SSGC_DIV GENMASK(4, 1) > > +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */ > > +#define OLB_PCSR1_SPREAD GENMASK(9, 5) > > +#define OLB_PCSR1_DIS_SSCG BIT(10) > > +/* Down-spread or center-spread */ > > +#define OLB_PCSR1_DOWN_SPREAD BIT(11) > > +#define OLB_PCSR1_FRAC_IN GENMASK(31, 12) > > + > > +static struct clk_hw_onecell_data *eq5c_clk_data; > > +static struct regmap *eq5c_olb; > > Drop these two. No file-scope regmaps for drivers. Use private container > structures. I wouldn't know how to handle the two steps then. Two clocks and the clk provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER(). The rest is at platform device probe. Without a static, there are no way to pass the struct clk_hw_onecell_data from one to the other. I've looked at all clock drivers that do CLK_OF_DECLARE_DRIVER() and register a platform driver. - The following use a static variable: drivers/clk/axis/clk-artpec6.c drivers/clk/clk-aspeed.c drivers/clk/clk-ast2600.c drivers/clk/clk-eyeq5.c drivers/clk/clk-gemini.c drivers/clk/clk-milbeaut.c drivers/clk/mediatek/clk-mt2701.c drivers/clk/mediatek/clk-mt6797.c drivers/clk/mediatek/clk-mt8173-infracfg.c drivers/clk/nxp/clk-lpc18xx-creg.c drivers/clk/ralink/clk-mt7621.c drivers/clk/ralink/clk-mtmips.c drivers/clk/sunxi/clk-mod0.c drivers/clk/axis/clk-artpec6.c - Those two declare different clock providers at init and probe: drivers/clk/ralink/clk-mt7621.c drivers/clk/sunxi/clk-mod0.c - It doesn't register new clocks at probe (only resets) so no need to share variables. drivers/clk/ralink/clk-mtmips.c > > ... > > > +static void __init eq5c_init(struct device_node *np) > > +{ > > + struct device_node *parent_np = of_get_parent(np); > > + int i, ret; > > + > > + eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS), > > + GFP_KERNEL); > > + if (!eq5c_clk_data) { > > + ret = -ENOMEM; > > + goto err; > > + } > > + > > + eq5c_clk_data->num = EQ5C_NB_CLKS; > > + > > + /* > > + * Mark all clocks as deferred. We register some now and others at > > + * platform device probe. > > + */ > > + for (i = 0; i < EQ5C_NB_CLKS; i++) > > + eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER); > > + > > + /* > > + * Currently, if OLB is not available, we log an error, fail init then > > How it could be not available? Only with broken initcall ordering. Fix > your initcall ordering and then simplify all this weird code. of_syscon_register() and regmap_init_mmio() lists many reasons for it to not be available. Am I missing something? Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 7:43 AM CET, Krzysztof Kozlowski wrote: > You miss here, in this place, the most important information which I > asked previously - dependencies/merging: > https://lore.kernel.org/all/db9b2786-29f7-4d45-9087-a9f85b770b6c@linaro.org/ Clearly I still forgot some dt-bindings dependencies. I now get what you meant at the time. Those explain the robot failures btw. Should it just be me saying "hey patches X depends on patches A, B, C?" Or are there even Git message trailers to mention dependencies? Care to give a patch link that does it the right way? Sorry about that, Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 8:00 AM CET, Krzysztof Kozlowski wrote: > On 23/01/2024 19:46, Théo Lebrun wrote: > > Add the Mobileye EyeQ5 reset controller driver. It belongs to a syscon > > region called OLB. It might grow to add later support of other > > platforms from Mobileye. [...] > > +static int _eq5r_busy_wait(struct eq5r_private *priv, struct device *dev, > > + u32 domain, u32 offset, bool assert) > > +{ > > + unsigned int val, mask; > > + int i; > > + > > + lockdep_assert_held(&priv->mutexes[domain]); > > + > > + switch (domain) { > > + case 0: > > + for (i = 0; i < D0_TIMEOUT_POLL; i++) { > > + regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val); > > + val = !(val & BIT(offset)); > > + if (val == assert) > > + return 0; > > + __udelay(1); > > What is even "__udelay"? It is the first use in drivers. Please use > common methods, like fsleep or udelay... but actually you should rather > use regmap_read_poll_timeout() or some variants instead of open-coding it. udelay is an alias to __udelay on MIPS, which is why this didn't look odd to me. Fixed. [...] > > +static void _eq5r_assert(struct eq5r_private *priv, u32 domain, u32 offset) > > Drop leading _ and name the function in some informative way. Fixed by turning `_eq5r_assert` into `eq5r_assert_withlock`, and co. [...] > > + > > +static int eq5r_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + struct device_node *parent_np = of_get_parent(np); > > + struct eq5r_private *priv; > > + int ret, i; > > + > > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > You leak parent. Fixed in all three clk+reset+pinctrl drivers. They all had this issue. > > > + > > + dev_set_drvdata(dev, priv); > > + > > + priv->olb = ERR_PTR(-ENODEV); > > + if (parent_np) { > > + priv->olb = syscon_node_to_regmap(parent_np); > > + of_node_put(parent_np); > > + } > > + if (IS_ERR(priv->olb)) > > Also here > > > + return PTR_ERR(priv->olb); > > This looks over-complicated. First, you cannot just > dev_get_regmap(pdev->dev.parent)? No dev_get_regmap() cannot be used as it doesn't pick up syscon regmaps. I've just tried it. However I've simplified the logic, it looks better now. static int eq5r_probe(struct platform_device *pdev) { struct device_node *parent_np; /* ... */ parent_np = of_get_parent(np); if (!parent_np) return -ENODEV; priv->olb = syscon_node_to_regmap(parent_np); of_node_put(parent_np); if (IS_ERR(priv->olb)) return PTR_ERR(priv->olb); /* ... */ } [...] > > +static struct platform_driver eq5r_driver = { > > + .probe = eq5r_probe, > > + .driver = { > > + .name = "eyeq5-reset", > > + .of_match_table = eq5r_match_table, > > + }, > > +}; > > + > > +static int __init eq5r_init(void) > > +{ > > + return platform_driver_register(&eq5r_driver); > > +} > > + > > +arch_initcall(eq5r_init); > > This is does not look like arch code, but driver or subsys. Use regular > module_driver. I see there is such pattern in reset but I doubt this is > something good. Indeed I've moved things to using the builtin_platform_driver() macro. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 8:03 AM CET, Krzysztof Kozlowski wrote: > On 23/01/2024 19:46, Théo Lebrun wrote: > > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later > > support of other platforms from Mobileye. It belongs to a syscon region > > called OLB. > > > > Existing pins and their function live statically in the driver code > > rather than in the devicetree, see compatible match data. > > > > ... > > > +static int eq5p_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + struct device_node *parent_np = of_get_parent(np); > > + const struct eq5p_match *match = of_device_get_match_data(dev); > > + struct pinctrl_dev *pctldev; > > + struct eq5p_pinctrl *pctrl; > > + int ret; > > + > > + pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL); > > + if (!pctrl) > > + return -ENOMEM; > > + > > + pctrl->olb = ERR_PTR(-ENODEV); > > + if (parent_np) > > + pctrl->olb = syscon_node_to_regmap(parent_np); > > + if (IS_ERR(pctrl->olb)) > > + pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb"); > > + if (IS_ERR(pctrl->olb)) > > + return PTR_ERR(pctrl->olb); > > No, we talked about this, you got comments on this. There is no > mobileye,olb. You cannot have undocumented properties. Clearly and I fully agree. It's a versioning issue on my side. It's been fixed (again, oops). > > > + > > + pctrl->regs = match->regs; > > + pctrl->funcs = match->funcs; > > + pctrl->nfuncs = match->nfuncs; > > + > > + pctrl->desc.name = dev_name(dev); > > + pctrl->desc.pins = match->pins; > > + pctrl->desc.npins = match->npins; > > + pctrl->desc.pctlops = &eq5p_pinctrl_ops; > > + pctrl->desc.pmxops = &eq5p_pinmux_ops; > > + pctrl->desc.confops = &eq5p_pinconf_ops; > > + pctrl->desc.owner = THIS_MODULE; > > + > > + ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev); > > + if (ret) { > > + dev_err(dev, "Failed registering pinctrl device: %d\n", ret); > > + return ret; > > + } > > + > > + ret = pinctrl_enable(pctldev); > > + if (ret) { > > + dev_err(dev, "Failed enabling pinctrl device: %d\n", ret); > > + return ret; > > + } > > + > > + dev_info(dev, "probed\n"); > > I am pretty sure you got comments for these. Drop such debugs from all > of your code. Current and future. Same thing, it must have been lost in the same fixup patch as the previous mistake. [...] > > +static struct platform_driver eq5p_driver = { > > + .driver = { > > + .name = "eyeq5-pinctrl", > > + .of_match_table = eq5p_match, > > + }, > > + .probe = eq5p_probe, > > +}; > > + > > +static int __init eq5p_init(void) > > +{ > > + return platform_driver_register(&eq5p_driver); > > +} > > +core_initcall(eq5p_init); > > No, pins are not a core_initcall. This could be arch_initcall, but > considering you depend on the parent this must be module driver. > > Even from this dependency point of view your initcalls are totally wrong > and will lead to issues. Same as reset: moved to using the builtin_platform_driver() macro. No need for it to be module as it cannot be one. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 9:33 AM CET, Sergei Shtylyov wrote: > On 1/23/24 9:46 PM, Théo Lebrun wrote: > > > Remove the `reg-io-width` property from the olb@e00000 syscon. The > > The diff says it's system-controller@e00000 now... :-) Oops, nice catch! Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, On Wed Jan 24, 2024 at 11:54 AM CET, Philipp Zabel wrote: > On Di, 2024-01-23 at 19:46 +0100, Théo Lebrun wrote: > [...] > > diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c > > new file mode 100644 > > index 000000000000..2217e42e140b > > --- /dev/null > > +++ b/drivers/reset/reset-eyeq5.c > > @@ -0,0 +1,383 @@ > [...] > > > +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id) > > +{ > > + struct eq5r_private *priv = dev_get_drvdata(rcdev->dev); > > rcdev is contained in priv, you can just use container_of instead of > chasing pointers around. That's right. Fixed with this local macro: #define rcdev_to_priv(rcdev) container_of(rcdev, struct eq5r_private, rcdev) > > + u32 offset = id & GENMASK(7, 0); > > + u32 domain = id >> 8; > > + int ret; > > + > > + if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT)) > > + return -EINVAL; > > Reset controls with domain >= EQ5R_DOMAIN_COUNT are already weeded out > during request by of_xlate, so this check is not necessary. It was some defensive programming. I've removed this precautionary condition from the places it appeared. > > > + dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset); > > + > > + mutex_lock(&priv->mutexes[domain]); > > + _eq5r_assert(priv, domain, offset); > > + ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, true); > > + mutex_unlock(&priv->mutexes[domain]); > > + > > + return ret; > > Consider using guard(mutex)(&priv->mutexes[domain]) from > linux/cleanup.h to automatically unlock on return. Done. I had never used that __cleanup attr feature. It simplifies returns. > > [...] > > +static int eq5r_reset(struct reset_controller_dev *rcdev, unsigned long id) > > Is this used by anything? If unused, I'd prefer this not to be > implemented. If it is used, is no delay required between assert and > deassert by any consumer? Not really, it follows what is done in the downstream vendor kernel. I've had a quick look in this kernel and I don't see any consumer of the API. For the moment I'll remove it. > > > +{ > > + struct device *dev = rcdev->dev; > > + struct eq5r_private *priv = dev_get_drvdata(dev); > > + u32 offset = id & GENMASK(7, 0); > > + u32 domain = id >> 8; > > + int ret; > > + > > + if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT)) > > + return -EINVAL; > > + > > + dev_dbg(dev, "%u-%u: reset request\n", domain, offset); > > + > > + mutex_lock(&priv->mutexes[domain]); > > + > > + _eq5r_assert(priv, domain, offset); > > + ret = _eq5r_busy_wait(priv, dev, domain, offset, true); > > + if (ret) /* don't let an error disappear silently */ > > + dev_warn(dev, "%u-%u: reset assert failed: %d\n", > > + domain, offset, ret); > > Why not return the error though? The goal was to still run through the deassert even if the assert returned an error. Goal was to address potential edge case of assert returning an error but still taking place, in which case we want to try to deassert to put the peripheral in a de-asserted state (as before the call). Not a concern anymore as the function is being removed. > > > + _eq5r_deassert(priv, domain, offset); > > + ret = _eq5r_busy_wait(priv, dev, domain, offset, false); > > + > > + mutex_unlock(&priv->mutexes[domain]); > > + > > + return ret; > > +} > [...] > > +static int eq5r_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + struct device_node *parent_np = of_get_parent(np); > > + struct eq5r_private *priv; > > + int ret, i; > > + > > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > Using devm_kzalloc() avoids leaking this on error return or driver > unbind. Done, thanks. > > > + if (!priv) > > + return -ENOMEM; > > + > > + dev_set_drvdata(dev, priv); > > + > > + priv->olb = ERR_PTR(-ENODEV); > > + if (parent_np) { > > + priv->olb = syscon_node_to_regmap(parent_np); > > + of_node_put(parent_np); > > + } > > + if (IS_ERR(priv->olb)) > > + return PTR_ERR(priv->olb); > > + > > + for (i = 0; i < EQ5R_DOMAIN_COUNT; i++) > > + mutex_init(&priv->mutexes[i]); > > + > > + priv->rcdev.ops = &eq5r_ops; > > + priv->rcdev.owner = THIS_MODULE; > > + priv->rcdev.dev = dev; > > + priv->rcdev.of_node = np; > > + priv->rcdev.of_reset_n_cells = 2; > > + priv->rcdev.of_xlate = eq5r_of_xlate; > > + > > + priv->rcdev.nr_resets = 0; > > + for (i = 0; i < EQ5R_DOMAIN_COUNT; i++) > > + priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]); > > + > > + ret = reset_controller_register(&priv->rcdev); > > Similarly, use devm_reset_controller_register() or disable driver > unbind with suppress_bind_attrs. Switched to the devres version, thanks. Thanks Philipp, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 4:19 PM CET, Rob Herring wrote: > On Tue, Jan 23, 2024 at 07:46:55PM +0100, Théo Lebrun wrote: > > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later > > support of other platforms from Mobileye. It belongs to a syscon region > > called OLB. > > > > Existing pins and their function live statically in the driver code > > rather than in the devicetree, see compatible match data. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- [...] > > diff --git a/drivers/pinctrl/pinctrl-eyeq5.c b/drivers/pinctrl/pinctrl-eyeq5.c [...] > > +static const struct eq5p_match eq5p_match_a = { > > + .regs = { > > + [EQ5P_PD] = 0x0C0, > > + [EQ5P_PU] = 0x0C4, > > + [EQ5P_DS_LOW] = 0x0D0, > > + [EQ5P_DS_HIGH] = 0x0D4, > > + [EQ5P_IOCR] = 0x0B0, > > + }, > > + .pins = eq5p_pins_a, > > + .npins = ARRAY_SIZE(eq5p_pins_a), > > + .funcs = eq5p_functions_a, > > + .nfuncs = ARRAY_SIZE(eq5p_functions_a), > > +}; > > + > > +static const struct eq5p_match eq5p_match_b = { > > + .regs = { > > + [EQ5P_PD] = 0x0C8, > > + [EQ5P_PU] = 0x0CC, > > + [EQ5P_DS_LOW] = 0x0D8, > > + [EQ5P_DS_HIGH] = 0x0DC, > > + [EQ5P_IOCR] = 0x0B4, > > + }, > > These are all the same relative offsets, so you really only need to > store the base offset. Indeed, and I don't think I had even noticed. Thanks. > The use of 2 compatibles is a bit questionable as the programming model > appears to be the same and only which pins differ. Surely there are > some other pinctrl drivers handling mutiple instances. I can confirm the programming model is the same across both banks. I've addressed your comment in my answer to yours on [PATCH v3 04/17]. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 24/01/2024 17:41, Théo Lebrun wrote: > Hello, > > On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote: >> On 23/01/2024 19:46, Théo Lebrun wrote: >>> Add the Mobileye EyeQ5 clock controller driver. It might grow to add >>> support for other platforms from Mobileye. >>> >>> It handles 10 read-only PLLs derived from the main crystal on board. It >>> exposes a table-based divider clock used for OSPI. Other platform >>> clocks are not configurable and therefore kept as fixed-factor >>> devicetree nodes. >>> >>> Two PLLs are required early on and are therefore registered at >>> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the >>> UARTs. >>> >> >> >>> +#define OLB_PCSR1_RESET BIT(0) >>> +#define OLB_PCSR1_SSGC_DIV GENMASK(4, 1) >>> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */ >>> +#define OLB_PCSR1_SPREAD GENMASK(9, 5) >>> +#define OLB_PCSR1_DIS_SSCG BIT(10) >>> +/* Down-spread or center-spread */ >>> +#define OLB_PCSR1_DOWN_SPREAD BIT(11) >>> +#define OLB_PCSR1_FRAC_IN GENMASK(31, 12) >>> + >>> +static struct clk_hw_onecell_data *eq5c_clk_data; >>> +static struct regmap *eq5c_olb; >> >> Drop these two. No file-scope regmaps for drivers. Use private container >> structures. > > I wouldn't know how to handle the two steps then. Two clocks and the clk > provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER(). Right, if some clocks have to be early, CLK_OF_DECLARE_DRIVER needs static ones. But your commit subject says it is a platform driver and all other pieces of this code is rather incompatible with this approach. Do not use CLK_OF_DECLARE_DRIVER for cases where you have dependencies because it forces you to manually order initcalls, which is exactly what we do not want. > The rest is at platform device probe. Without a static, there are no > way to pass the struct clk_hw_onecell_data from one to the other. > > I've looked at all clock drivers that do CLK_OF_DECLARE_DRIVER() and > register a platform driver. Even though the code is correct, using arguments "other did it" will not work. You want to say that you implement legacy, poor code because you saw legacy, poor code? > > - The following use a static variable: > drivers/clk/axis/clk-artpec6.c > drivers/clk/clk-aspeed.c > drivers/clk/clk-ast2600.c > drivers/clk/clk-eyeq5.c > drivers/clk/clk-gemini.c > drivers/clk/clk-milbeaut.c > drivers/clk/mediatek/clk-mt2701.c > drivers/clk/mediatek/clk-mt6797.c > drivers/clk/mediatek/clk-mt8173-infracfg.c > drivers/clk/nxp/clk-lpc18xx-creg.c > drivers/clk/ralink/clk-mt7621.c > drivers/clk/ralink/clk-mtmips.c > drivers/clk/sunxi/clk-mod0.c > drivers/clk/axis/clk-artpec6.c > > - Those two declare different clock providers at init and probe: > drivers/clk/ralink/clk-mt7621.c > drivers/clk/sunxi/clk-mod0.c > > - It doesn't register new clocks at probe (only resets) so no need to > share variables. > drivers/clk/ralink/clk-mtmips.c > >> >> ... >> >>> +static void __init eq5c_init(struct device_node *np) >>> +{ >>> + struct device_node *parent_np = of_get_parent(np); >>> + int i, ret; >>> + >>> + eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS), >>> + GFP_KERNEL); >>> + if (!eq5c_clk_data) { >>> + ret = -ENOMEM; >>> + goto err; >>> + } >>> + >>> + eq5c_clk_data->num = EQ5C_NB_CLKS; >>> + >>> + /* >>> + * Mark all clocks as deferred. We register some now and others at >>> + * platform device probe. >>> + */ >>> + for (i = 0; i < EQ5C_NB_CLKS; i++) >>> + eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER); >>> + >>> + /* >>> + * Currently, if OLB is not available, we log an error, fail init then >> >> How it could be not available? Only with broken initcall ordering. Fix >> your initcall ordering and then simplify all this weird code. > > of_syscon_register() and regmap_init_mmio() lists many reasons for > it to not be available. Am I missing something? Yes, initcall ordering. Best regards, Krzysztof
Hi, On Thu Jan 25, 2024 at 8:46 AM CET, Krzysztof Kozlowski wrote: > On 24/01/2024 17:41, Théo Lebrun wrote: > > Hello, > > > > On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote: > >> On 23/01/2024 19:46, Théo Lebrun wrote: > >>> Add the Mobileye EyeQ5 clock controller driver. It might grow to add > >>> support for other platforms from Mobileye. > >>> > >>> It handles 10 read-only PLLs derived from the main crystal on board. It > >>> exposes a table-based divider clock used for OSPI. Other platform > >>> clocks are not configurable and therefore kept as fixed-factor > >>> devicetree nodes. > >>> > >>> Two PLLs are required early on and are therefore registered at > >>> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the > >>> UARTs. > >>> > >> > >> > >>> +#define OLB_PCSR1_RESET BIT(0) > >>> +#define OLB_PCSR1_SSGC_DIV GENMASK(4, 1) > >>> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */ > >>> +#define OLB_PCSR1_SPREAD GENMASK(9, 5) > >>> +#define OLB_PCSR1_DIS_SSCG BIT(10) > >>> +/* Down-spread or center-spread */ > >>> +#define OLB_PCSR1_DOWN_SPREAD BIT(11) > >>> +#define OLB_PCSR1_FRAC_IN GENMASK(31, 12) > >>> + > >>> +static struct clk_hw_onecell_data *eq5c_clk_data; > >>> +static struct regmap *eq5c_olb; > >> > >> Drop these two. No file-scope regmaps for drivers. Use private container > >> structures. > > > > I wouldn't know how to handle the two steps then. Two clocks and the clk > > provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER(). > > Right, if some clocks have to be early, CLK_OF_DECLARE_DRIVER needs > static ones. But your commit subject says it is a platform driver and > all other pieces of this code is rather incompatible with this approach. That is my bad on the commit subject. What do you refer to by "all other pieces of this code is rather incompatible with this approach"? I've tried to minimise the use of static variables. Therefore as soon as the probe is started, we switch to the usual way of using a private struct that contains our info. > > Do not use CLK_OF_DECLARE_DRIVER for cases where you have dependencies > because it forces you to manually order initcalls, which is exactly what > we do not want. What should I be using? I got confirmation from Stephen that this mixture of CLK_OF_DECLARE_DRIVER() + platform driver is what I should be using as review in my V1. https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.org/ > > > > The rest is at platform device probe. Without a static, there are no > > way to pass the struct clk_hw_onecell_data from one to the other. > > > > I've looked at all clock drivers that do CLK_OF_DECLARE_DRIVER() and > > register a platform driver. > > Even though the code is correct, using arguments "other did it" will not > work. You want to say that you implement legacy, poor code because you > saw legacy, poor code? Yes I see what you mean. It's just that this is not the sort of things that are documented. And learning The Right Way(TM) when you don't know it can only be done by looking at existing stuff. I'm being exhaustive to avoid basing my approach on one old-school driver that is using the wrong approach. > >> ... > >> > >>> +static void __init eq5c_init(struct device_node *np) > >>> +{ > >>> + struct device_node *parent_np = of_get_parent(np); > >>> + int i, ret; > >>> + > >>> + eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS), > >>> + GFP_KERNEL); > >>> + if (!eq5c_clk_data) { > >>> + ret = -ENOMEM; > >>> + goto err; > >>> + } > >>> + > >>> + eq5c_clk_data->num = EQ5C_NB_CLKS; > >>> + > >>> + /* > >>> + * Mark all clocks as deferred. We register some now and others at > >>> + * platform device probe. > >>> + */ > >>> + for (i = 0; i < EQ5C_NB_CLKS; i++) > >>> + eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER); > >>> + > >>> + /* > >>> + * Currently, if OLB is not available, we log an error, fail init then > >> > >> How it could be not available? Only with broken initcall ordering. Fix > >> your initcall ordering and then simplify all this weird code. > > > > of_syscon_register() and regmap_init_mmio() lists many reasons for > > it to not be available. Am I missing something? > > Yes, initcall ordering. You said the regmap can only not be available with broken initcall ordering. I say that is not the only reason. About initcall, I've removed those that used initcall in the three drivers I'm using except this clk one that requires two clocks at of_clk_init(). Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 25/01/2024 12:53, Théo Lebrun wrote: > Hi, > > On Thu Jan 25, 2024 at 8:46 AM CET, Krzysztof Kozlowski wrote: >> On 24/01/2024 17:41, Théo Lebrun wrote: >>> Hello, >>> >>> On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote: >>>> On 23/01/2024 19:46, Théo Lebrun wrote: >>>>> Add the Mobileye EyeQ5 clock controller driver. It might grow to add >>>>> support for other platforms from Mobileye. >>>>> >>>>> It handles 10 read-only PLLs derived from the main crystal on board. It >>>>> exposes a table-based divider clock used for OSPI. Other platform >>>>> clocks are not configurable and therefore kept as fixed-factor >>>>> devicetree nodes. >>>>> >>>>> Two PLLs are required early on and are therefore registered at >>>>> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the >>>>> UARTs. >>>>> >>>> >>>> >>>>> +#define OLB_PCSR1_RESET BIT(0) >>>>> +#define OLB_PCSR1_SSGC_DIV GENMASK(4, 1) >>>>> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */ >>>>> +#define OLB_PCSR1_SPREAD GENMASK(9, 5) >>>>> +#define OLB_PCSR1_DIS_SSCG BIT(10) >>>>> +/* Down-spread or center-spread */ >>>>> +#define OLB_PCSR1_DOWN_SPREAD BIT(11) >>>>> +#define OLB_PCSR1_FRAC_IN GENMASK(31, 12) >>>>> + >>>>> +static struct clk_hw_onecell_data *eq5c_clk_data; >>>>> +static struct regmap *eq5c_olb; >>>> >>>> Drop these two. No file-scope regmaps for drivers. Use private container >>>> structures. >>> >>> I wouldn't know how to handle the two steps then. Two clocks and the clk >>> provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER(). >> >> Right, if some clocks have to be early, CLK_OF_DECLARE_DRIVER needs >> static ones. But your commit subject says it is a platform driver and >> all other pieces of this code is rather incompatible with this approach. > > That is my bad on the commit subject. What do you refer to by "all other > pieces of this code is rather incompatible with this approach"? That you depend on syscon. If it was regular MMIO block in SoC space, then no problem. If you depend on anything else providing you regmap, then any initcall ordering is fragile and error-prone. Avoid. > > I've tried to minimise the use of static variables. Therefore as soon as > the probe is started, we switch to the usual way of using a private > struct that contains our info. > >> >> Do not use CLK_OF_DECLARE_DRIVER for cases where you have dependencies >> because it forces you to manually order initcalls, which is exactly what >> we do not want. > > What should I be using? I got confirmation from Stephen that this > mixture of CLK_OF_DECLARE_DRIVER() + platform driver is what I should > be using as review in my V1. > > https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.org/ I see. In such case I believe it is error on relying on syscon. Best regards, Krzysztof
Hi, The goal of this series is to add clk, reset and pinctrl support for the Mobileye EyeQ5 platform [0]. Control of those is grouped inside a system controller block called "OLB". About clocks, we replaced the 10 fixed clocks from the initial platform support series [0] by 10 read-only fixed-factor PLLs provided by our clock driver. We also provide one table-based divider clock for OSPI. Two PLLs (for GIC timer & UARTs) are required at of_clk_init() so those are registered first, the rest comes at platform device probe. Resets are split in three domains, all dealt with by the same device. They have some behavior differences: - We busy-wait on the first two for hardware LBIST reasons (logic built-in self-test). - Domains 0 & 2 work in a bit-per-reset fashion while domain 1 works in a register-per-reset fashion. Pin control is about controlling bias, drive strength and muxing. The latter allows two functions per pin; the first function is always GPIO while the second one is pin-dependent. There exists two banks, each handled in a separate driver instance. Each pin maps to one pin group. That makes pin & group indexes the same, simplifying logic. This series inherits from the clk V2 [1], reset V1 [2] and pinctrl V1 [3]. Those were unified to simplify handling of dt-bindings. It is based on the series "[PATCH v6 00/15] Add support for the Mobileye EyeQ5 SoC" [0] rebased onto v6.8-rc1. Here is the patch list, split by subsystems: - clk: - [PATCH V3 01/17] clk: fixed-factor: add optional accuracy support - [PATCH V3 02/17] clk: fixed-factor: add fwname-based constructor functions - [PATCH V3 05/17] dt-bindings: clock: mobileye,eyeq5-clk: add bindings - [PATCH V3 08/17] clk: eyeq5: add platform driver - pinctrl: - [PATCH V3 03/17] dt-bindings: pinctrl: allow pin controller device without unit address - [PATCH V3 07/17] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings - [PATCH V3 10/17] pinctrl: eyeq5: add platform driver - MIPS: (note: dependent on the [0] series) - [PATCH V3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller - [PATCH V3 11/17] MIPS: mobileye: eyeq5: rename olb@e00000 to system-controller@e00000 - [PATCH V3 12/17] MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon - [PATCH V3 13/17] MIPS: mobileye: eyeq5: use OLB clocks controller - [PATCH V3 14/17] MIPS: mobileye: eyeq5: add OLB reset controller node - [PATCH V3 15/17] MIPS: mobileye: eyeq5: add reset properties to uarts - [PATCH V3 16/17] MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes - [PATCH V3 17/17] MIPS: mobileye: eyeq5: add pinctrl properties to UART nodes - reset: - [PATCH V3 06/17] dt-bindings: reset: mobileye,eyeq5-reset: add bindings - [PATCH V3 09/17] reset: eyeq5: add platform driver Thanks to Krzysztof, Rob & Stephen for the previous feedback! Have a nice day, Théo Lebrun [0]: https://lore.kernel.org/lkml/20240118155252.397947-1-gregory.clement@bootlin.com/ [1]: https://lore.kernel.org/lkml/20231227-mbly-clk-v2-0-a05db63c380f@bootlin.com/ [2]: https://lore.kernel.org/lkml/20231218-mbly-reset-v1-0-b4688b916213@bootlin.com/ [3]: https://lore.kernel.org/lkml/20231218-mbly-pinctrl-v1-0-2f7d366c2051@bootlin.com/ Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Changes in v3: - Unified the three series into one. - clk: split driver into two for clocks registered at of_clk_init() and clocks registered at platform device probe. - reset/bindings: drop reset dt-bindings header & add comment in driver to document known valid resets in each domain. - pinctrl/bindings: fix pinctrl.yaml to allow non unit addresses for pin controller devices. - all/bindings: remove possibility to use `mobileye,olb` phandle to get syscon. All three drivers use their parent node as syscon/regmap. - MIPS/bindings: fix bindings for OLB. Have single example in parent, removing all examples in child. - all: drop the "probed" logs. - Link to v2: https://lore.kernel.org/r/20231227-mbly-clk-v2-0-a05db63c380f@bootlin.com Changes in v2: - Drop [PATCH 1/5] that was taken by Stephen for clk-next. - Add accuracy support to fixed-factor that is enabled with a flag. Register prototypes were added to exploit this feature. - Add fw_name support to fixed-factor. This allows pointing to parent clocks using the value in `clock-names` in the DT. Register prototypes were added for that. - Bindings were modified to be less dumb: a binding was added for OLB and the clock-controller is a child property of it. Removed the possibility of pointing to OLB using a phandle. $nodename is the generic `clock-controller` and not custom `clocks`. Fix dt-bindings examples. - Fix commit message for the driver patch. Add details, remove useless fluff. - Squash both driver commits together. - Declare a platform_driver instead of using CLK_OF_DECLARE_DRIVER. This also means using `dev_*` for logging, removing `pr_fmt`. We add a pointer to device in the private structure. - Use fixed-factor instead of fixed-rate for PLLs. We don't grab a reference to the parent clk, instead using newly added fixed-factor register prototypes and fwname. - NULL is not an error when registering PLLs anymore. - Now checking the return value of of_clk_add_hw_provider for errors. - Fix includes. - Remove defensive conditional at start of eq5c_pll_parse_registers. - Rename clk_hw_to_ospi_priv to clk_to_priv to avoid confusion: it is not part of the clk_hw_* family of symbols. - Fix negative returns in eq5c_ospi_div_set_rate. It was a typo highlighted by Stephen Boyd. - Declare eq5c_ospi_div_ops as static. - In devicetree, move the OLB node prior to the UARTs, as platform device probe scheduling is dependent on devicetree ordering. This is required to declare the driver as a platform driver, else it CLK_OF_DECLARE_DRIVER is required. - In device, create a core0-timer-clk fixed clock to feed to the GIC timer. It requires a clock earlier than platform bus type init. - Link to v1: https://lore.kernel.org/r/20231218-mbly-clk-v1-0-44ce54108f06@bootlin.com --- Théo Lebrun (17): clk: fixed-factor: add optional accuracy support clk: fixed-factor: add fwname-based constructor functions dt-bindings: pinctrl: allow pin controller device without unit address dt-bindings: soc: mobileye: add EyeQ5 OLB system controller dt-bindings: clock: mobileye,eyeq5-clk: add bindings dt-bindings: reset: mobileye,eyeq5-reset: add bindings dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings clk: eyeq5: add platform driver reset: eyeq5: add platform driver pinctrl: eyeq5: add platform driver MIPS: mobileye: eyeq5: rename olb@e00000 to system-controller@e00000 MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon MIPS: mobileye: eyeq5: use OLB clocks controller MIPS: mobileye: eyeq5: add OLB reset controller node MIPS: mobileye: eyeq5: add reset properties to uarts MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes MIPS: mobileye: eyeq5: add pinctrl properties to UART nodes .../bindings/clock/mobileye,eyeq5-clk.yaml | 41 ++ .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 77 +++ .../devicetree/bindings/pinctrl/pinctrl.yaml | 18 +- .../bindings/reset/mobileye,eyeq5-reset.yaml | 32 ++ .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 77 +++ MAINTAINERS | 8 + .../{eyeq5-fixed-clocks.dtsi => eyeq5-clocks.dtsi} | 54 +- arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi | 128 +++++ arch/mips/boot/dts/mobileye/eyeq5.dtsi | 37 +- drivers/clk/Kconfig | 11 + drivers/clk/Makefile | 1 + drivers/clk/clk-eyeq5.c | 414 ++++++++++++++ drivers/clk/clk-fixed-factor.c | 103 +++- drivers/pinctrl/Kconfig | 15 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-eyeq5.c | 595 +++++++++++++++++++++ drivers/reset/Kconfig | 12 + drivers/reset/Makefile | 1 + drivers/reset/reset-eyeq5.c | 383 +++++++++++++ include/dt-bindings/clock/mobileye,eyeq5-clk.h | 22 + include/linux/clk-provider.h | 26 +- 21 files changed, 1995 insertions(+), 61 deletions(-) --- base-commit: 84f23245916391a55be31e37e48cea4da085b100 change-id: 20231023-mbly-clk-87ce5c241f08 Best regards,