Message-ID: <b9b8aa19-aa08-42ff-8b2f-dedb1e64700c@linaro.org>
Date: Wed, 24 Jan 2024 08:00:35 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
 Gregory CLEMENT <gregory.clement@...tlin.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
 Linus Walleij <linus.walleij@...aro.org>, Rafał Miłecki
 <rafal@...ecki.pl>, Philipp Zabel <p.zabel@...gutronix.de>
Cc: Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
 linux-mips@...r.kernel.org, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Tawfik Bayouk <tawfik.bayouk@...ileye.com>, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 09/17] reset: eyeq5: add platform driver

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@...tlin.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

Powered by blists - more mailing lists