| Message ID | 20240628-mbly-reset-v1-0-2a8294fd4392@bootlin.com |
|---|---|
| Headers | show |
| Series | Add Mobileye EyeQ reset support | expand |
Hi Théo, On Fr, 2024-06-28 at 18:11 +0200, Théo Lebrun wrote: > Add Mobileye EyeQ reset controller driver, for EyeQ5, EyeQ6L and EyeQ6H > SoCs. Instances belong to a shared register region called OLB and gets > spawned as auxiliary device to the platform driver for clock. > > There is one OLB instance for EyeQ5 and EyeQ6L. There are seven OLB > instances on EyeQ6H; three have a reset controller embedded: > - West and east get handled by the same compatible. > - Acc (accelerator) is another one. > > Each instance vary in the number and types of reset domains. > Instances with single domain expect a single cell, others two. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/reset/Kconfig | 13 ++ > drivers/reset/Makefile | 1 + > drivers/reset/reset-eyeq.c | 562 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 576 insertions(+) > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 7112f5932609..14f3f4af0b10 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -66,6 +66,19 @@ config RESET_BRCMSTB_RESCAL > This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on > BCM7216. > > +config RESET_EYEQ > + bool "Mobileye EyeQ reset controller" > + depends on AUXILIARY_BUS This should select AUXILIARY_BUS instead. > + depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST > + default MACH_EYEQ5 || MACH_EYEQ6H > + help > + This enables the Mobileye EyeQ reset controller, used in EyeQ5, EyeQ6L > + and EyeQ6H SoCs. > + > + It has one or more domains, with a varying number of resets in each. > + Registers are located in a shared register region called OLB. EyeQ6H > + has multiple reset instances. > + > config RESET_GPIO > tristate "GPIO reset controller" > help > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index fd8b49fa46fc..a4e6fea29800 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_EYEQ) += reset-eyeq.o > obj-$(CONFIG_RESET_GPIO) += reset-gpio.o > obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o > obj-$(CONFIG_RESET_IMX7) += reset-imx7.o > diff --git a/drivers/reset/reset-eyeq.c b/drivers/reset/reset-eyeq.c > new file mode 100644 > index 000000000000..9717572ac4b3 > --- /dev/null > +++ b/drivers/reset/reset-eyeq.c > @@ -0,0 +1,562 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Reset driver for the Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms. > + * > + * Controllers live in a shared register region called OLB. EyeQ5 and EyeQ6L > + * have a single OLB instance for a single reset controller. EyeQ6H has seven > + * OLB instances; three host reset controllers. > + * > + * Each reset controller has one or more domain. Domains are of a given type > + * (see enum eqr_domain_type), with a valid offset mask (up to 32 resets per > + * domain). > + * > + * Domain types define expected behavior: one-register-per-reset, > + * one-bit-per-reset, status detection method, busywait duration, etc. > + * > + * We use eqr_ as prefix, as-in "EyeQ Reset", but way shorter. > + * > + * Known resets in EyeQ5 domain 0 (type EQR_EYEQ5_SARCR): > + * 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 EyeQ5 domain 1 (type EQR_EYEQ5_ACRP): > + * 0. VMP0 1. VMP1 2. VMP2 3. VMP3 > + * 4. PMA0 5. PMA1 6. PMAC0 7. PMAC1 > + * 8. MPC0 9. MPC1 10. MPC2 11. MPC3 > + * 12. MPC4 > + * > + * Known resets in EyeQ5 domain 2 (type EQR_EYEQ5_PCIE): > + * 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 > + * > + * Known resets in EyeQ6L domain 0 (type EQR_EYEQ5_SARCR): > + * 0. SPI0 1. SPI1 2. UART0 3. I2C0 > + * 4. I2C1 5. TIMER0 6. TIMER1 7. TIMER2 > + * 8. TIMER3 9. WD0 10. WD1 11. EXT0 > + * 12. EXT1 13. GPIO > + * > + * Known resets in EyeQ6L domain 1 (type EQR_EYEQ5_ACRP): > + * 0. VMP0 1. VMP1 2. VMP2 3. VMP3 > + * 4. PMA0 5. PMA1 6. PMAC0 7. PMAC1 > + * 8. MPC0 9. MPC1 10. MPC2 11. MPC3 > + * 12. MPC4 > + * > + * Known resets in EyeQ6H west/east (type EQR_EYEQ6H_SARCR): > + * 0. CAN 1. SPI0 2. SPI1 3. UART0 > + * 4. UART1 5. I2C0 6. I2C1 7. -hole- > + * 8. TIMER0 9. TIMER1 10. WD 11. EXT TIMER > + * 12. GPIO > + * > + * Known resets in EyeQ6H acc (type EQR_EYEQ5_ACRP): > + * 1. XNN0 2. XNN1 3. XNN2 4. XNN3 > + * 5. VMP0 6. VMP1 7. VMP2 8. VMP3 > + * 9. PMA0 10. PMA1 11. MPC0 12. MPC1 > + * 13. MPC2 14. MPC3 15. PERIPH > + * > + * Abbreviations: > + * - PMA: Programmable Macro Array > + * - MPC: Multi-threaded Processing Clusters > + * - VMP: Vector Microcode Processors > + * > + * Copyright (C) 2024 Mobileye Vision Technologies Ltd. > + */ > + > +#include <linux/array_size.h> > +#include <linux/auxiliary_bus.h> > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/bug.h> > +#include <linux/cleanup.h> > +#include <linux/container_of.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/errno.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/iopoll.h> > +#include <linux/lockdep.h> > +#include <linux/mod_devicetable.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/reset-controller.h> > +#include <linux/slab.h> > +#include <linux/types.h> > + > +/* > + * A reset ID, as returned by eqr_of_xlate_*(), is a (domain, offset) pair. > + * Low byte is domain, rest is offset. > + */ > +#define ID_DOMAIN_MASK GENMASK(7, 0) > +#define ID_OFFSET_MASK GENMASK(31, 8) > + > +enum eqr_domain_type { > + EQR_EYEQ5_SARCR, > + EQR_EYEQ5_ACRP, > + EQR_EYEQ5_PCIE, > + EQR_EYEQ6H_SARCR, > +}; > + > +/* > + * Domain type EQR_EYEQ5_SARCR register offsets. > + */ > +#define EQR_EYEQ5_SARCR_REQUEST (0x000) > +#define EQR_EYEQ5_SARCR_STATUS (0x004) > + > +/* > + * Domain type EQR_EYEQ5_ACRP register masks. > + * Registers are: base + 4 * offset. > + */ > +#define EQR_EYEQ5_ACRP_PD_REQ BIT(0) > +#define EQR_EYEQ5_ACRP_ST_POWER_DOWN BIT(27) > +#define EQR_EYEQ5_ACRP_ST_ACTIVE BIT(29) > + > +/* > + * Domain type EQR_EYEQ6H_SARCR register offsets. > + */ > +#define EQR_EYEQ6H_SARCR_RST_REQUEST (0x000) > +#define EQR_EYEQ6H_SARCR_CLK_STATUS (0x004) > +#define EQR_EYEQ6H_SARCR_RST_STATUS (0x008) > +#define EQR_EYEQ6H_SARCR_CLK_REQUEST (0x00C) > + > +struct eqr_busy_wait_timings { > + unsigned long sleep_us; > + unsigned long timeout_us; > +}; > + > +static const struct eqr_busy_wait_timings eqr_timings[] = { > + [EQR_EYEQ5_SARCR] = {1, 10}, > + [EQR_EYEQ5_ACRP] = {1, 40 * USEC_PER_MSEC}, /* LBIST implies long timeout. */ > + /* EQR_EYEQ5_PCIE does no busy waiting. */ > + [EQR_EYEQ6H_SARCR] = {1, 400}, > +}; > + > +#define EQR_MAX_DOMAIN_COUNT 3 > + > +struct eqr_domain_descriptor { > + enum eqr_domain_type type; > + u32 valid_mask; > + unsigned int offset; > +}; > + > +struct eqr_match_data { > + unsigned int domain_count; > + const struct eqr_domain_descriptor *domains; > +}; > + > +struct eqr_private { > + struct mutex mutexes[EQR_MAX_DOMAIN_COUNT]; Is there any benefit from per-domain mutexes over just a single mutex? > + void __iomem *base; > + const struct eqr_match_data *data; > + struct reset_controller_dev rcdev; > +}; > + > +#define rcdev_to_priv(rcdev) container_of(rcdev, struct eqr_private, rcdev) Please use checkpatch --strict, and ideally mention when you ignore a warning on purpose. In this case, the macro parameter should named something else, because the last parameter to container_of must be "rcdev" verbatim. This only works by accident because the passed parameter also happens to be called called "rcdev" at all call sites. > +static u32 eqr_double_readl(void __iomem *addr_a, void __iomem *addr_b, > + u32 *dest_a, u32 *dest_b) > +{ > + *dest_a = readl(addr_a); > + *dest_b = readl(addr_b); > + return 0; /* read_poll_timeout() op argument must return something. */ > +} > + > +static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev, > + u32 domain, u32 offset, bool assert) > +{ > + enum eqr_domain_type domain_type = priv->data->domains[domain].type; > + unsigned long sleep_us, timeout_us; > + u32 val, mask, val0, val1; > + void __iomem *base, *reg; > + int ret; > + > + lockdep_assert_held(&priv->mutexes[domain]); > + > + base = priv->base + priv->data->domains[domain].offset; > + sleep_us = eqr_timings[domain_type].sleep_us; > + timeout_us = eqr_timings[domain_type].timeout_us; You can initialize these at the declaration. > + > + switch (domain_type) { > + case EQR_EYEQ5_SARCR: > + reg = base + EQR_EYEQ5_SARCR_STATUS; > + mask = BIT(offset); > + > + ret = readl_poll_timeout(reg, val, !(val & mask) == assert, > + sleep_us, timeout_us); > + break; > + > + case EQR_EYEQ5_ACRP: > + reg = base + 4 * offset; > + if (assert) > + mask = EQR_EYEQ5_ACRP_ST_POWER_DOWN; > + else > + mask = EQR_EYEQ5_ACRP_ST_ACTIVE; > + > + ret = readl_poll_timeout(reg, val, !!(val & mask), > + sleep_us, timeout_us); > + break; > + > + case EQR_EYEQ5_PCIE: > + ret = 0; /* No busy waiting. */ > + break; > + > + case EQR_EYEQ6H_SARCR: > + /* > + * Wait until both bits change: > + * readl(base + EQR_EYEQ6H_SARCR_RST_STATUS) & BIT(offset) > + * readl(base + EQR_EYEQ6H_SARCR_CLK_STATUS) & BIT(offset) > + */ > + mask = BIT(offset); > + ret = read_poll_timeout(eqr_double_readl, val, > + (!(val0 & mask) == assert) && > + (!(val1 & mask) == assert), I'd remove a level of indentation here. > + sleep_us, timeout_us, false, > + base + EQR_EYEQ6H_SARCR_RST_STATUS, > + base + EQR_EYEQ6H_SARCR_CLK_STATUS, > + &val0, &val1); Calling these variables something like rst_status and clk_status, would make this a bit easier to parse. > + break; > + > + default: > + WARN_ON(1); > + ret = -EINVAL; > + break; > + } > + > + if (ret == -ETIMEDOUT) > + dev_dbg(dev, "%u-%u: timeout\n", domain, offset); > + return ret; > +} > + > +static void eqr_assert_locked(struct eqr_private *priv, u32 domain, u32 offset) > +{ > + enum eqr_domain_type domain_type = priv->data->domains[domain].type; > + void __iomem *base, *reg; > + u32 val; > + > + lockdep_assert_held(&priv->mutexes[domain]); > + > + base = priv->base + priv->data->domains[domain].offset; > + > + switch (domain_type) { > + case EQR_EYEQ5_SARCR: > + reg = base + EQR_EYEQ5_SARCR_REQUEST; > + writel(readl(reg) & ~BIT(offset), reg); > + break; > + > + case EQR_EYEQ5_ACRP: > + reg = base + 4 * offset; > + writel(readl(reg) | EQR_EYEQ5_ACRP_PD_REQ, reg); > + break; > + > + case EQR_EYEQ5_PCIE: > + writel(readl(base) & ~BIT(offset), base); > + break; > + > + case EQR_EYEQ6H_SARCR: > + val = readl(base + EQR_EYEQ6H_SARCR_RST_REQUEST); > + val &= ~BIT(offset); > + writel(val, base + EQR_EYEQ6H_SARCR_RST_REQUEST); > + writel(val, base + EQR_EYEQ6H_SARCR_CLK_REQUEST); This looks peculiar. Why is it ok to write the value read from RST_REQUEST into CLK_REQUEST? > + break; > + > + default: > + WARN_ON(1); > + break; > + } > +} > + > +static int eqr_assert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct eqr_private *priv = rcdev_to_priv(rcdev); > + u32 domain = FIELD_GET(ID_DOMAIN_MASK, id); > + u32 offset = FIELD_GET(ID_OFFSET_MASK, id); > + > + dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset); > + > + guard(mutex)(&priv->mutexes[domain]); > + > + eqr_assert_locked(priv, domain, offset); > + return eqr_busy_wait_locked(priv, rcdev->dev, domain, offset, true); > +} > + > +static void eqr_deassert_locked(struct eqr_private *priv, u32 domain, > + u32 offset) > +{ > + enum eqr_domain_type domain_type = priv->data->domains[domain].type; > + void __iomem *base, *reg; > + u32 val; > + > + lockdep_assert_held(&priv->mutexes[domain]); > + > + base = priv->base + priv->data->domains[domain].offset; > + > + switch (domain_type) { > + case EQR_EYEQ5_SARCR: > + reg = base + EQR_EYEQ5_SARCR_REQUEST; > + writel(readl(reg) | BIT(offset), reg); > + break; > + > + case EQR_EYEQ5_ACRP: > + reg = base + 4 * offset; > + writel(readl(reg) & ~EQR_EYEQ5_ACRP_PD_REQ, reg); > + break; > + > + case EQR_EYEQ5_PCIE: > + writel(readl(base) | BIT(offset), base); > + break; > + > + case EQR_EYEQ6H_SARCR: > + val = readl(base + EQR_EYEQ6H_SARCR_RST_REQUEST); > + val |= BIT(offset); > + writel(val, base + EQR_EYEQ6H_SARCR_RST_REQUEST); > + writel(val, base + EQR_EYEQ6H_SARCR_CLK_REQUEST); > + break; > + > + default: > + WARN_ON(1); > + break; > + } > +} > + > +static int eqr_deassert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct eqr_private *priv = rcdev_to_priv(rcdev); > + u32 domain = FIELD_GET(ID_DOMAIN_MASK, id); > + u32 offset = FIELD_GET(ID_OFFSET_MASK, id); > + > + dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset); > + > + guard(mutex)(&priv->mutexes[domain]); > + > + eqr_deassert_locked(priv, domain, offset); > + return eqr_busy_wait_locked(priv, rcdev->dev, domain, offset, false); > +} > + > +static int eqr_status(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + u32 domain = FIELD_GET(ID_DOMAIN_MASK, id); > + struct eqr_private *priv = rcdev_to_priv(rcdev); > + enum eqr_domain_type domain_type = priv->data->domains[domain].type; > + u32 offset = FIELD_GET(ID_OFFSET_MASK, id); I'd put domain and offset declaration next to each other. regards Philipp
Hello Philipp, On Mon Jul 1, 2024 at 10:59 AM CEST, Philipp Zabel wrote: > On Fr, 2024-06-28 at 18:11 +0200, Théo Lebrun wrote: > > Add Mobileye EyeQ reset controller driver, for EyeQ5, EyeQ6L and EyeQ6H > > SoCs. Instances belong to a shared register region called OLB and gets > > spawned as auxiliary device to the platform driver for clock. > > > > There is one OLB instance for EyeQ5 and EyeQ6L. There are seven OLB > > instances on EyeQ6H; three have a reset controller embedded: > > - West and east get handled by the same compatible. > > - Acc (accelerator) is another one. > > > > Each instance vary in the number and types of reset domains. > > Instances with single domain expect a single cell, others two. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/reset/Kconfig | 13 ++ > > drivers/reset/Makefile | 1 + > > drivers/reset/reset-eyeq.c | 562 +++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 576 insertions(+) > > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > > index 7112f5932609..14f3f4af0b10 100644 > > --- a/drivers/reset/Kconfig > > +++ b/drivers/reset/Kconfig > > @@ -66,6 +66,19 @@ config RESET_BRCMSTB_RESCAL > > This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on > > BCM7216. > > > > +config RESET_EYEQ > > + bool "Mobileye EyeQ reset controller" > > + depends on AUXILIARY_BUS > > This should > > select AUXILIARY_BUS > > instead. Done, looking like this now: config RESET_EYEQ bool "Mobileye EyeQ reset controller" depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST select AUXILIARY_BUS default MACH_EYEQ5 || MACH_EYEQ6H help ... [...] > > +#define EQR_MAX_DOMAIN_COUNT 3 > > + > > +struct eqr_domain_descriptor { > > + enum eqr_domain_type type; > > + u32 valid_mask; > > + unsigned int offset; > > +}; > > + > > +struct eqr_match_data { > > + unsigned int domain_count; > > + const struct eqr_domain_descriptor *domains; > > +}; > > + > > +struct eqr_private { > > + struct mutex mutexes[EQR_MAX_DOMAIN_COUNT]; > > Is there any benefit from per-domain mutexes over just a single mutex? Some domains can stay locked for pretty long in theory because of Logic built-in self-test (LBIST). This is the reasoning behind eqr_busy_wait_locked(). Other domains are not concerned by this behavior. More concretely, on EyeQ5, SARCR and ACRP domains can lock their mutexes for a relatively long time, and for good reasons. We wouldn't want the PCIE domain to suffer from that if it happens to (de)assert a reset at the same time. > > > + void __iomem *base; > > + const struct eqr_match_data *data; > > + struct reset_controller_dev rcdev; > > +}; > > + > > +#define rcdev_to_priv(rcdev) container_of(rcdev, struct eqr_private, rcdev) > > Please use checkpatch --strict, and ideally mention when you ignore a > warning on purpose. In this case, the macro parameter should named > something else, because the last parameter to container_of must be > "rcdev" verbatim. This only works by accident because the passed > parameter also happens to be called called "rcdev" at all call sites. I have let this CHECK from `checkpatch --strict` slip through indeed. Other remaining messages, with explanations, are: - WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? This is done in a single patch [0] in the MIPS series to avoid conflicts in between series. - CHECK: struct mutex definition without comment This is about the above mutexes field. Do you want a code comment about the reasoning for one mutex per domain? - WARNING: DT compatible string "[...]" appears un-documented Bindings are added in a single commit in the MIPS series [1], to avoid conflicts. > > > +static u32 eqr_double_readl(void __iomem *addr_a, void __iomem *addr_b, > > + u32 *dest_a, u32 *dest_b) > > +{ > > + *dest_a = readl(addr_a); > > + *dest_b = readl(addr_b); > > + return 0; /* read_poll_timeout() op argument must return something. */ > > +} > > + > > +static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev, > > + u32 domain, u32 offset, bool assert) > > +{ > > + enum eqr_domain_type domain_type = priv->data->domains[domain].type; > > + unsigned long sleep_us, timeout_us; > > + u32 val, mask, val0, val1; > > + void __iomem *base, *reg; > > + int ret; > > + > > + lockdep_assert_held(&priv->mutexes[domain]); > > + > > + base = priv->base + priv->data->domains[domain].offset; > > + sleep_us = eqr_timings[domain_type].sleep_us; > > + timeout_us = eqr_timings[domain_type].timeout_us; > > You can initialize these at the declaration. Done, declarations will look like: static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev, u32 domain, u32 offset, bool assert) { void __iomem *base = priv->base + priv->data->domains[domain].offset; enum eqr_domain_type domain_type = priv->data->domains[domain].type; unsigned long timeout_us = eqr_timings[domain_type].timeout_us; unsigned long sleep_us = eqr_timings[domain_type].sleep_us; u32 val, mask, val0, val1; void __iomem *reg; int ret; // ... } > > > + > > + switch (domain_type) { > > + case EQR_EYEQ5_SARCR: > > + reg = base + EQR_EYEQ5_SARCR_STATUS; > > + mask = BIT(offset); > > + > > + ret = readl_poll_timeout(reg, val, !(val & mask) == assert, > > + sleep_us, timeout_us); > > + break; > > + > > + case EQR_EYEQ5_ACRP: > > + reg = base + 4 * offset; > > + if (assert) > > + mask = EQR_EYEQ5_ACRP_ST_POWER_DOWN; > > + else > > + mask = EQR_EYEQ5_ACRP_ST_ACTIVE; > > + > > + ret = readl_poll_timeout(reg, val, !!(val & mask), > > + sleep_us, timeout_us); > > + break; > > + > > + case EQR_EYEQ5_PCIE: > > + ret = 0; /* No busy waiting. */ > > + break; > > + > > + case EQR_EYEQ6H_SARCR: > > + /* > > + * Wait until both bits change: > > + * readl(base + EQR_EYEQ6H_SARCR_RST_STATUS) & BIT(offset) > > + * readl(base + EQR_EYEQ6H_SARCR_CLK_STATUS) & BIT(offset) > > + */ > > + mask = BIT(offset); > > + ret = read_poll_timeout(eqr_double_readl, val, > > + (!(val0 & mask) == assert) && > > + (!(val1 & mask) == assert), > > I'd remove a level of indentation here. Indeed! > > + sleep_us, timeout_us, false, > > + base + EQR_EYEQ6H_SARCR_RST_STATUS, > > + base + EQR_EYEQ6H_SARCR_CLK_STATUS, > > + &val0, &val1); > > Calling these variables something like rst_status and clk_status, would > make this a bit easier to parse. It now looks like: static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev, u32 domain, u32 offset, bool assert) { // ... switch (domain_type) { // ... case EQR_EYEQ6H_SARCR: /* * Wait until both bits change: * readl(base + EQR_EYEQ6H_SARCR_RST_STATUS) & BIT(offset) * readl(base + EQR_EYEQ6H_SARCR_CLK_STATUS) & BIT(offset) */ mask = BIT(offset); ret = read_poll_timeout(eqr_double_readl, val, (!(rst_status & mask) == assert) && (!(clk_status & mask) == assert), sleep_us, timeout_us, false, base + EQR_EYEQ6H_SARCR_RST_STATUS, base + EQR_EYEQ6H_SARCR_CLK_STATUS, &rst_status, &clk_status); break; // ... } // ... } > > > + break; > > + > > + default: > > + WARN_ON(1); > > + ret = -EINVAL; > > + break; > > + } > > + > > + if (ret == -ETIMEDOUT) > > + dev_dbg(dev, "%u-%u: timeout\n", domain, offset); > > + return ret; > > +} > > + > > +static void eqr_assert_locked(struct eqr_private *priv, u32 domain, u32 offset) > > +{ > > + enum eqr_domain_type domain_type = priv->data->domains[domain].type; > > + void __iomem *base, *reg; > > + u32 val; > > + > > + lockdep_assert_held(&priv->mutexes[domain]); > > + > > + base = priv->base + priv->data->domains[domain].offset; > > + > > + switch (domain_type) { > > + case EQR_EYEQ5_SARCR: > > + reg = base + EQR_EYEQ5_SARCR_REQUEST; > > + writel(readl(reg) & ~BIT(offset), reg); > > + break; > > + > > + case EQR_EYEQ5_ACRP: > > + reg = base + 4 * offset; > > + writel(readl(reg) | EQR_EYEQ5_ACRP_PD_REQ, reg); > > + break; > > + > > + case EQR_EYEQ5_PCIE: > > + writel(readl(base) & ~BIT(offset), base); > > + break; > > + > > + case EQR_EYEQ6H_SARCR: > > + val = readl(base + EQR_EYEQ6H_SARCR_RST_REQUEST); > > + val &= ~BIT(offset); > > + writel(val, base + EQR_EYEQ6H_SARCR_RST_REQUEST); > > + writel(val, base + EQR_EYEQ6H_SARCR_CLK_REQUEST); > > This looks peculiar. Why is it ok to write the value read from > RST_REQUEST into CLK_REQUEST? What is abstracted away by the hardware on EyeQ5 is not anymore on EyeQ6H. Previously a single register was used for requests and a single register for status. Now there are two request registers and two status registers. Those registers *must be kept in sync*. The register name referencing clock is not to be confused with the clock driver of the system-controller. It is describing a register within the reset controller. This hardware interface is odd, I might add a comment? [...] > > +static int eqr_status(struct reset_controller_dev *rcdev, unsigned long id) > > +{ > > + u32 domain = FIELD_GET(ID_DOMAIN_MASK, id); > > + struct eqr_private *priv = rcdev_to_priv(rcdev); > > + enum eqr_domain_type domain_type = priv->data->domains[domain].type; > > + u32 offset = FIELD_GET(ID_OFFSET_MASK, id); > > I'd put domain and offset declaration next to each other. Done: static int eqr_status(struct reset_controller_dev *rcdev, unsigned long id) { u32 domain = FIELD_GET(ID_DOMAIN_MASK, id); u32 offset = FIELD_GET(ID_OFFSET_MASK, id); struct eqr_private *priv = rcdev_to_priv(rcdev); enum eqr_domain_type domain_type = priv->data->domains[domain].type; void __iomem *base, *reg; // ... } I'll be sending a new revision in the week with those fixes. Thanks Philipp, [0]: https://lore.kernel.org/lkml/20240628-mbly-mips-v1-3-f53f5e4c422b@bootlin.com/ [1]: https://lore.kernel.org/lkml/20240628-mbly-mips-v1-1-f53f5e4c422b@bootlin.com/ -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Théo, On Mo, 2024-07-01 at 18:19 +0200, Théo Lebrun wrote: [...] > > Is there any benefit from per-domain mutexes over just a single mutex? > > Some domains can stay locked for pretty long in theory because of Logic > built-in self-test (LBIST). This is the reasoning behind > eqr_busy_wait_locked(). > > Other domains are not concerned by this behavior. > > More concretely, on EyeQ5, SARCR and ACRP domains can lock their mutexes > for a relatively long time, and for good reasons. We wouldn't want the > PCIE domain to suffer from that if it happens to (de)assert a reset at > the same time. Thank you for the explanation. > > > > > + void __iomem *base; > > > + const struct eqr_match_data *data; > > > + struct reset_controller_dev rcdev; > > > +}; > > > + > > > +#define rcdev_to_priv(rcdev) container_of(rcdev, struct eqr_private, rcdev) > > > > Please use checkpatch --strict, and ideally mention when you ignore a > > warning on purpose. In this case, the macro parameter should named > > something else, because the last parameter to container_of must be > > "rcdev" verbatim. This only works by accident because the passed > > parameter also happens to be called called "rcdev" at all call sites. Thinking about this again, it would be even better to turn this into a static inline function instead. > I have let this CHECK from `checkpatch --strict` slip through indeed. > Other remaining messages, with explanations, are: > > - WARNING: added, moved or deleted file(s), does MAINTAINERS need > updating? > > This is done in a single patch [0] in the MIPS series to avoid > conflicts in between series. > > - CHECK: struct mutex definition without comment > > This is about the above mutexes field. Do you want a code comment > about the reasoning for one mutex per domain? Yes, that would be nice. I'm not pedantic about the lock comments because in reset drivers it's usually pretty obvious what the lock is used for, but mentioning that the mutexes cover register read-modify- write plus waiting for LBIST on some domains seems like a good idea. [...] > > > > > +static void eqr_assert_locked(struct eqr_private *priv, u32 domain, u32 offset) > > > +{ [...] > > > + case EQR_EYEQ6H_SARCR: > > > + val = readl(base + EQR_EYEQ6H_SARCR_RST_REQUEST); > > > + val &= ~BIT(offset); > > > + writel(val, base + EQR_EYEQ6H_SARCR_RST_REQUEST); > > > + writel(val, base + EQR_EYEQ6H_SARCR_CLK_REQUEST); > > > > This looks peculiar. Why is it ok to write the value read from > > RST_REQUEST into CLK_REQUEST? > > What is abstracted away by the hardware on EyeQ5 is not anymore on > EyeQ6H. Previously a single register was used for requests and a single > register for status. Now there are two request registers and two status > registers. > > Those registers *must be kept in sync*. The register name referencing > clock is not to be confused with the clock driver of the > system-controller. It is describing a register within the reset > controller. > > This hardware interface is odd, I might add a comment? Yes, please. With the knowledge that those registers must be kept in sync, this goes from strange to obvious. regards Philipp
Hello Philipp, On Tue Jul 2, 2024 at 11:19 AM CEST, Philipp Zabel wrote: > On Mo, 2024-07-01 at 18:19 +0200, Théo Lebrun wrote: [...] > > > > +#define rcdev_to_priv(rcdev) container_of(rcdev, struct eqr_private, rcdev) > > > > > > Please use checkpatch --strict, and ideally mention when you ignore a > > > warning on purpose. In this case, the macro parameter should named > > > something else, because the last parameter to container_of must be > > > "rcdev" verbatim. This only works by accident because the passed > > > parameter also happens to be called called "rcdev" at all call sites. > > Thinking about this again, it would be even better to turn this into a > static inline function instead. I thought about it but checking drivers/pinctrl/ it looked like macros were more common for container_of() encapsulation. I'll go the static inline function. Plain, simple: static inline struct eqr_private *eqr_rcdev_to_priv(struct reset_controller_dev *x) { return container_of(x, struct eqr_private, rcdev); } > > > I have let this CHECK from `checkpatch --strict` slip through indeed. > > Other remaining messages, with explanations, are: > > > > - WARNING: added, moved or deleted file(s), does MAINTAINERS need > > updating? > > > > This is done in a single patch [0] in the MIPS series to avoid > > conflicts in between series. > > > > - CHECK: struct mutex definition without comment > > > > This is about the above mutexes field. Do you want a code comment > > about the reasoning for one mutex per domain? > > Yes, that would be nice. I'm not pedantic about the lock comments > because in reset drivers it's usually pretty obvious what the lock is > used for, but mentioning that the mutexes cover register read-modify- > write plus waiting for LBIST on some domains seems like a good idea. Sure: struct eqr_private { /* * One mutex per domain for read-modify-write operations on registers. * Some domains can be involved in LBIST which implies long critical * sections; we wouldn't want other domains to be impacted by that. */ struct mutex mutexes[EQR_MAX_DOMAIN_COUNT]; void __iomem *base; const struct eqr_match_data *data; struct reset_controller_dev rcdev; }; > > [...] > > > > > > > +static void eqr_assert_locked(struct eqr_private *priv, u32 domain, u32 offset) > > > > +{ > [...] > > > > + case EQR_EYEQ6H_SARCR: > > > > + val = readl(base + EQR_EYEQ6H_SARCR_RST_REQUEST); > > > > + val &= ~BIT(offset); > > > > + writel(val, base + EQR_EYEQ6H_SARCR_RST_REQUEST); > > > > + writel(val, base + EQR_EYEQ6H_SARCR_CLK_REQUEST); > > > > > > This looks peculiar. Why is it ok to write the value read from > > > RST_REQUEST into CLK_REQUEST? > > > > What is abstracted away by the hardware on EyeQ5 is not anymore on > > EyeQ6H. Previously a single register was used for requests and a single > > register for status. Now there are two request registers and two status > > registers. > > > > Those registers *must be kept in sync*. The register name referencing > > clock is not to be confused with the clock driver of the > > system-controller. It is describing a register within the reset > > controller. > > > > This hardware interface is odd, I might add a comment? > > Yes, please. With the knowledge that those registers must be kept in > sync, this goes from strange to obvious. Done, I added a plain comment on both assert and deassert: /* RST_REQUEST and CLK_REQUEST must be kept in sync. */ Thanks Philipp, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
This is a new iteration on the Mobileye system-controller series [0]. It has been split into separate series to facilitate merging. This series adds a platform driver handling SoC controllers resets. It is an auxiliary driver being instantiated by the platform clk driver. We support EyeQ5, EyeQ6L and EyeQ6H SoCs. The last one is special in that there are seven instances of this system-controller. Three of those (west, east, acc) contain a reset section. Related series are targeted at clk [1], pinctrl [2] and MIPS [3]. Have a nice day, Théo [0]: https://lore.kernel.org/lkml/20240620-mbly-olb-v3-0-5f29f8ca289c@bootlin.com/ [1]: https://lore.kernel.org/lkml/20240628-mbly-clk-v1-0-edb1e29ea4c1@bootlin.com/ [2]: https://lore.kernel.org/lkml/20240628-mbly-pinctrl-v1-0-c878192d6b0a@bootlin.com/ [3]: https://lore.kernel.org/lkml/20240628-mbly-mips-v1-0-f53f5e4c422b@bootlin.com/ Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Changes since OLB v3 [0]: - MAINTAINERS: Move changes into a separate commit to avoid merge conflicts. This commit is in the MIPS series [3]. - dt-bindings: Take Reviewed-by: Rob Herring. - Kconfig: do not depend on COMMON_CLK_EYEQ. This symbol is not defined in this series, it is defined in the clk series [1]. - Kconfig: do depend on AUXILIARY_BUS. - Kconfig: remove outdated "depends on MFD_SYSCON". - driver: remove "#include <linux/platform_device.h>". - driver: cast platdata to (void _iomem *) explicitely. --- Théo Lebrun (2): Revert "dt-bindings: reset: mobileye,eyeq5-reset: add bindings" reset: eyeq: add platform driver .../bindings/reset/mobileye,eyeq5-reset.yaml | 43 -- drivers/reset/Kconfig | 13 + drivers/reset/Makefile | 1 + drivers/reset/reset-eyeq.c | 562 +++++++++++++++++++++ 4 files changed, 576 insertions(+), 43 deletions(-) --- base-commit: f2661062f16b2de5d7b6a5c42a9a5c96326b8454 change-id: 20240628-mbly-reset-ac6c66d3bf95 Best regards,