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

  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(+)
  

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 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
  

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
  

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