diff mbox series

[v8,04/10] reset: eyeq5: add platform driver

Message ID 20240227-mbly-clk-v8-4-c57fbda7664a@bootlin.com (mailing list archive)
State Superseded, archived
Headers show
Series Add support for Mobileye EyeQ5 system controller | expand

Commit Message

Théo Lebrun Feb. 27, 2024, 2:55 p.m. UTC
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>
---
 drivers/reset/Kconfig       |  12 ++
 drivers/reset/Makefile      |   1 +
 drivers/reset/reset-eyeq5.c | 342 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)

Comments

Andy Shevchenko Feb. 27, 2024, 5:27 p.m. UTC | #1
On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 reset controller driver. It belongs to a syscon
> region called OLB. It might grow to add later support of other
> platforms from Mobileye.

...

The inclusion block is a semi-random. Please, follow IWYU principle.

+ array_size.h
+ bits.h
+ bug.h
+ container_of.h

> +#include <linux/cleanup.h>
> +#include <linux/delay.h>

+ device.h
+ err.h
+ io.h
+ lockdep.h

+ mod_devicetable.h

> +#include <linux/mutex.h>

> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>

Are all of them in use?

> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>

+ types.h

...

> +/* Vendor-provided timeout values. D1 has a long timeout because of LBIST. */
> +#define D0_TIMEOUT_POLL			10
> +#define D1_TIMEOUT_POLL			40000

We use units as suffixes. The above doesn't tell magnitude of timeouts.

Also constants like USEC_PER_MSEC are useful to make code robust.

...

> +	unsigned int val, mask;
> +	int i;
> +
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		for (i = 0; i < D0_TIMEOUT_POLL; i++) {
> +			val = readl(priv->base_d0 + D0_SARCR1);
> +			val = !(val & BIT(offset));
> +			if (val == assert)
> +				return 0;
> +			udelay(1);
> +		}
> +		break;

See what iopoll.h gives you.

> +	case 1:
> +		mask = assert ? D1_ACRP_ST_POWER_DOWN : D1_ACRP_ST_ACTIVE;
> +		for (i = 0; i < D1_TIMEOUT_POLL; i++) {
> +			val = readl(priv->base_d1 + 4 * offset);
> +			if (val & mask)
> +				return 0;
> +			udelay(1);
> +		}

Ditto.

> +		break;
> +	case 2:
> +		return 0; /* No busy waiting for domain 2. */
> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}

> +	dev_dbg(dev, "%u-%u: timeout\n", domain, offset);

> +	return -ETIMEDOUT;

...

> +static void eq5r_assert_withlock(struct eq5r_private *priv, u32 domain,
> +				 u32 offset)
> +{
> +	void __iomem *reg;
> +
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		reg = priv->base_d0 + D0_SARCR0;
> +		writel(readl(reg) & ~BIT(offset), reg);
> +		break;
> +	case 1:
> +		reg = priv->base_d1 + 4 * offset;
> +		writel(readl(reg) | D1_ACRP_PD_REQ, reg);
> +		break;
> +	case 2:
> +		reg = priv->base_d2;
> +		writel(readl(reg) & ~BIT(offset), reg);
> +		break;
> +	default:
> +		WARN_ON(1);

break;

> +	}
> +}
> +
> +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = rcdev_to_priv(rcdev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;
> +
> +	dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
> +
> +	guard(mutex)(&priv->mutexes[domain]);
> +	eq5r_assert_withlock(priv, domain, offset);
> +	return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, true);
> +}
> +
> +static void eq5r_deassert_withlock(struct eq5r_private *priv, u32 domain,
> +				   u32 offset)
> +{
> +	void __iomem *reg;
> +
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		reg = priv->base_d0 + D0_SARCR0;
> +		writel(readl(reg) | BIT(offset), reg);
> +		break;
> +	case 1:
> +		reg = priv->base_d1 + 4 * offset;
> +		writel(readl(reg) & ~D1_ACRP_PD_REQ, reg);
> +		break;
> +	case 2:
> +		reg = priv->base_d2;
> +		writel(readl(reg) | BIT(offset), reg);
> +		break;
> +	default:
> +		WARN_ON(1);

break;

> +	}
> +}

...

> +static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = rcdev_to_priv(rcdev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;

Perhaps

	u32 offset = (id & GENMASK(7, 0)) >> 0;
	u32 domain = (id & GENMASK(31, 8)) >> 8;

for better understanding the split?

> +	dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset);
> +
> +	guard(mutex)(&priv->mutexes[domain]);
> +	eq5r_deassert_withlock(priv, domain, offset);
> +	return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, false);

withlock is not usual naming pattern, "locked" is shorter and widely used.

> +}
> +
> +static int eq5r_status(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = rcdev_to_priv(rcdev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;

Ditto.

> +	u32 val;
> +
> +	dev_dbg(rcdev->dev, "%u-%u: status request\n", domain, offset);
> +
> +	guard(mutex)(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		val = readl(priv->base_d0 + D0_SARCR1);
> +		return !(val & BIT(offset));
> +	case 1:
> +		val = readl(priv->base_d1 + 4 * offset);
> +		return !(val & D1_ACRP_ST_ACTIVE);
> +	case 2:
> +		val = readl(priv->base_d2);
> +		return !(val & BIT(offset));
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +	struct eq5r_private *priv;
> +	int ret, i;

Why is i signed?

> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base_d0 = devm_platform_ioremap_resource_byname(pdev, "d0");
> +	if (IS_ERR(priv->base_d0))
> +		return PTR_ERR(priv->base_d0);
> +
> +	priv->base_d1 = devm_platform_ioremap_resource_byname(pdev, "d1");
> +	if (IS_ERR(priv->base_d1))
> +		return PTR_ERR(priv->base_d1);
> +
> +	priv->base_d2 = devm_platform_ioremap_resource_byname(pdev, "d2");
> +	if (IS_ERR(priv->base_d2))
> +		return PTR_ERR(priv->base_d2);
> +
> +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> +		mutex_init(&priv->mutexes[i]);
> +
> +	priv->rcdev.ops = &eq5r_ops;
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.dev = dev;

> +	priv->rcdev.of_node = np;

It's better to use device_set_node().

> +	priv->rcdev.of_reset_n_cells = 2;
> +	priv->rcdev.of_xlate = eq5r_of_xlate;
> +
> +	priv->rcdev.nr_resets = 0;
> +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)

> +		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);

Please, use corresponding hweightXX() API.

> +	ret = devm_reset_controller_register(dev, &priv->rcdev);
> +	if (ret) {

> +		dev_err(dev, "Failed registering reset controller: %d\n", ret);
> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	return 0;
> +}

...

> +static struct platform_driver eq5r_driver = {
> +	.probe = eq5r_probe,
> +	.driver = {
> +		.name = "eyeq5-reset",
> +		.of_match_table = eq5r_match_table,
> +	},
> +};

> +

Unneeded blank line.

> +builtin_platform_driver(eq5r_driver);
Théo Lebrun Feb. 28, 2024, 5:04 p.m. UTC | #2
Hello,

I won't be answering to straight forward comments.

On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 reset controller driver. It belongs to a syscon
> > region called OLB. It might grow to add later support of other
> > platforms from Mobileye.
>
> ...
>
> The inclusion block is a semi-random. Please, follow IWYU principle.
>
> + array_size.h
> + bits.h
> + bug.h
> + container_of.h
>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
>
> + device.h
> + err.h
> + io.h
> + lockdep.h
>
> + mod_devicetable.h
>
> > +#include <linux/mutex.h>
>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
>
> Are all of them in use?
>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
>
> + types.h

I'm adding yours + errno.h + init.h (for THIS_MODULE) + slab.h (for GFP
flags). I'm removing unused of_address.h and of_device.h.

delay.h will be removed and iopoll.h will be added based on changes
following your comments.

[...]

> > +static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +	struct eq5r_private *priv = rcdev_to_priv(rcdev);
> > +	u32 offset = id & GENMASK(7, 0);
> > +	u32 domain = id >> 8;
>
> Perhaps
>
> 	u32 offset = (id & GENMASK(7, 0)) >> 0;
> 	u32 domain = (id & GENMASK(31, 8)) >> 8;
>
> for better understanding the split?

Do the additional zero-bit-shift and GENMASK() help understanding? My
brain needs time to parse them to then notice they do nothing and
simplify the code in my head, back to the original version.

I personally like the simplest version (the original one). But otherwise
FIELD_GET() with two globally-defined masks could be a solution as
well. I still prefer the original version better. Less symbols, less
complexity.

[...]

> > +	struct eq5r_private *priv;
> > +	int ret, i;
>
> Why is i signed?

No reason, will switch to unsigned int.

>
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->base_d0 = devm_platform_ioremap_resource_byname(pdev, "d0");
> > +	if (IS_ERR(priv->base_d0))
> > +		return PTR_ERR(priv->base_d0);
> > +
> > +	priv->base_d1 = devm_platform_ioremap_resource_byname(pdev, "d1");
> > +	if (IS_ERR(priv->base_d1))
> > +		return PTR_ERR(priv->base_d1);
> > +
> > +	priv->base_d2 = devm_platform_ioremap_resource_byname(pdev, "d2");
> > +	if (IS_ERR(priv->base_d2))
> > +		return PTR_ERR(priv->base_d2);
> > +
> > +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> > +		mutex_init(&priv->mutexes[i]);
> > +
> > +	priv->rcdev.ops = &eq5r_ops;
> > +	priv->rcdev.owner = THIS_MODULE;
> > +	priv->rcdev.dev = dev;
>
> > +	priv->rcdev.of_node = np;
>
> It's better to use device_set_node().

I don't see how device_set_node() can help? It works on struct device
pointers. Here priv->rcdev is a reset_controller_dev struct. There are
no users of device_set_node() in drivers/reset/.

>
> > +	priv->rcdev.of_reset_n_cells = 2;
> > +	priv->rcdev.of_xlate = eq5r_of_xlate;
> > +
> > +	priv->rcdev.nr_resets = 0;
> > +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
>
> > +		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
>
> Please, use corresponding hweightXX() API.

Noted. I did not find this keyword even though I searched quite a bit
for it. "popcount" sounds more logical to me. :-)

Thanks for the review Andy!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Andy Shevchenko Feb. 29, 2024, 11:22 a.m. UTC | #3
On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:

...

> > > +	u32 offset = id & GENMASK(7, 0);
> > > +	u32 domain = id >> 8;
> >
> > Perhaps
> >
> > 	u32 offset = (id & GENMASK(7, 0)) >> 0;
> > 	u32 domain = (id & GENMASK(31, 8)) >> 8;
> >
> > for better understanding the split?
> 
> Do the additional zero-bit-shift and GENMASK() help understanding? My
> brain needs time to parse them to then notice they do nothing and
> simplify the code in my head, back to the original version.

In my opinion yes, as you exactly showing the split.
But. The better is to use FIELD_GET().

> I personally like the simplest version (the original one). But otherwise
> FIELD_GET() with two globally-defined masks could be a solution as
> well.

Oh, yes, that's what just came to my mind without even looking here.

> I still prefer the original version better. Less symbols, less
> complexity.

[...]

> > > +	priv->rcdev.of_node = np;
> >
> > It's better to use device_set_node().
> 
> I don't see how device_set_node() can help? It works on struct device
> pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> no users of device_set_node() in drivers/reset/.

No users doesn't mean it's good. The API is relatively "new" and takes
care of two things:
1) it uses agnostic interface;
2) it doesn't require any firmware node direct dereference.

The 2) is most important here as allows us to refactor (firmware node) code
in the future.

> > > +	priv->rcdev.of_reset_n_cells = 2;
> > > +	priv->rcdev.of_xlate = eq5r_of_xlate;

However, ideally these should be also translated to use fwnode as IIO did,
for example.

...

> > > +		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
> >
> > Please, use corresponding hweightXX() API.
> 
> Noted. I did not find this keyword even though I searched quite a bit
> for it. "popcount" sounds more logical to me. :-)

Hmm... But it's fundamental, it's called Hamming weight.
https://en.wikipedia.org/wiki/Hamming_weight
Théo Lebrun Feb. 29, 2024, 12:18 p.m. UTC | #4
Hello,

On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:
>
> ...
>
> > > > +	u32 offset = id & GENMASK(7, 0);
> > > > +	u32 domain = id >> 8;
> > >
> > > Perhaps
> > >
> > > 	u32 offset = (id & GENMASK(7, 0)) >> 0;
> > > 	u32 domain = (id & GENMASK(31, 8)) >> 8;
> > >
> > > for better understanding the split?
> > 
> > Do the additional zero-bit-shift and GENMASK() help understanding? My
> > brain needs time to parse them to then notice they do nothing and
> > simplify the code in my head, back to the original version.
>
> In my opinion yes, as you exactly showing the split.
> But. The better is to use FIELD_GET().

I'll go with the FIELD_GET() option!

[...]

>
> > > > +	priv->rcdev.of_node = np;
> > >
> > > It's better to use device_set_node().
> > 
> > I don't see how device_set_node() can help? It works on struct device
> > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > no users of device_set_node() in drivers/reset/.
>
> No users doesn't mean it's good. The API is relatively "new" and takes
> care of two things:
> 1) it uses agnostic interface;
> 2) it doesn't require any firmware node direct dereference.
>
> The 2) is most important here as allows us to refactor (firmware node) code
> in the future.

I think I get the point of device_set_node(). I still do not understand
how it could help me fill the ->of_node field in a reset_controller_dev
structure?

Should I be using device_set_node() to fill the struct device pointer
and the reset subsystem, by some magic, will pick this up and use it
for its own of_node field? I've not seen any magic/code doing that.

[...]

> > > > +		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
> > >
> > > Please, use corresponding hweightXX() API.
> >
> > Noted. I did not find this keyword even though I searched quite a bit
> > for it. "popcount" sounds more logical to me. :-)
>
> Hmm... But it's fundamental, it's called Hamming weight.
> https://en.wikipedia.org/wiki/Hamming_weight

Makes sense now. I've always called it population count following the
name of the matching instruction on x86 (and I believe other ISAs). TIL.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Andy Shevchenko Feb. 29, 2024, 1:48 p.m. UTC | #5
On Thu, Feb 29, 2024 at 01:18:08PM +0100, Théo Lebrun wrote:
> On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:

...

> > > > > +	priv->rcdev.of_node = np;
> > > >
> > > > It's better to use device_set_node().
> > > 
> > > I don't see how device_set_node() can help? It works on struct device
> > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > no users of device_set_node() in drivers/reset/.
> >
> > No users doesn't mean it's good. The API is relatively "new" and takes
> > care of two things:
> > 1) it uses agnostic interface;
> > 2) it doesn't require any firmware node direct dereference.
> >
> > The 2) is most important here as allows us to refactor (firmware node) code
> > in the future.
> 
> I think I get the point of device_set_node(). I still do not understand
> how it could help me fill the ->of_node field in a reset_controller_dev
> structure?

Exactly why I put the above comment as recommendation. And then I elaborated
that entire reset framework should rather move towards fwnode.

> Should I be using device_set_node() to fill the struct device pointer
> and the reset subsystem, by some magic, will pick this up and use it
> for its own of_node field? I've not seen any magic/code doing that.

At bare minimum it will give beneficial things:
1) less burden in the drivers conversion in case fwnode happens (and I believe
   it's just matter of time) in reset framework;
2) hiding fwnode/of_node implemetation details (which is currently is layering
   violation to some extend (as we have a lot of *of_*() APIs to avoid direct
   access to of_node field in struct device).

The downside is that you will need to include property.h for this only thing.
And I don't see other code that can be converted to fwnode right away here.
Andy Shevchenko Feb. 29, 2024, 1:53 p.m. UTC | #6
On Thu, Feb 29, 2024 at 03:48:59PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 01:18:08PM +0100, Théo Lebrun wrote:

...

> The downside is that you will need to include property.h for this only thing.
> And I don't see other code that can be converted to fwnode right away here.

I meant here

	device_set_node(..., dev_fwnode(parent));

On the second thought it can survive probably without it in a form

	device_set_node(..., of_fwnode_handle(parent->of_node));

but this does not fully solve the fundamental problem with accessing of_node.
Théo Lebrun Feb. 29, 2024, 3:23 p.m. UTC | #7
Hello,

On Thu Feb 29, 2024 at 2:48 PM CET, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 01:18:08PM +0100, Théo Lebrun wrote:
> > On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:
>
> ...
>
> > > > > > +	priv->rcdev.of_node = np;
> > > > >
> > > > > It's better to use device_set_node().
> > > > 
> > > > I don't see how device_set_node() can help? It works on struct device
> > > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > > no users of device_set_node() in drivers/reset/.
> > >
> > > No users doesn't mean it's good. The API is relatively "new" and takes
> > > care of two things:
> > > 1) it uses agnostic interface;
> > > 2) it doesn't require any firmware node direct dereference.
> > >
> > > The 2) is most important here as allows us to refactor (firmware node) code
> > > in the future.
> > 
> > I think I get the point of device_set_node(). I still do not understand
> > how it could help me fill the ->of_node field in a reset_controller_dev
> > structure?
>
> Exactly why I put the above comment as recommendation. And then I elaborated
> that entire reset framework should rather move towards fwnode.

OK now I get it. One question: would using fwnode abstractions make
sense for a driver that is devicetree-only, and will stay that way?

However this sounds out-of-scope of such a driver addition. I also am
not familiar enough (yet?) with the reset subsystem and/or fwnode to be
able to bring this kind of changes upstream.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Philipp Zabel Feb. 29, 2024, 3:28 p.m. UTC | #8
On Do, 2024-02-29 at 15:48 +0200, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 01:18:08PM +0100, Théo Lebrun wrote:
> > On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:
> 
> ...
> 
> > > > > > +	priv->rcdev.of_node = np;
> > > > > 
> > > > > It's better to use device_set_node().
> > > > 
> > > > I don't see how device_set_node() can help? It works on struct device
> > > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > > no users of device_set_node() in drivers/reset/.
> > > 
> > > No users doesn't mean it's good. The API is relatively "new" and takes
> > > care of two things:
> > > 1) it uses agnostic interface;
> > > 2) it doesn't require any firmware node direct dereference.
> > > 
> > > The 2) is most important here as allows us to refactor (firmware node) code
> > > in the future.
> > 
> > I think I get the point of device_set_node(). I still do not understand
> > how it could help me fill the ->of_node field in a reset_controller_dev
> > structure?
> 
> Exactly why I put the above comment as recommendation. And then I elaborated
> that entire reset framework should rather move towards fwnode.

For context, there have been initial patches for this, that turned out
not to be necessary later on:

https://lore.kernel.org/lkml/20220324141237.297207-1-clement.leger@bootlin.com/

At this point, there still is no real use case for non-DT reset
controls on the horizon.

> 
regards
Philipp
Andy Shevchenko Feb. 29, 2024, 3:33 p.m. UTC | #9
On Thu, Feb 29, 2024 at 04:23:01PM +0100, Théo Lebrun wrote:
> On Thu Feb 29, 2024 at 2:48 PM CET, Andy Shevchenko wrote:
> > On Thu, Feb 29, 2024 at 01:18:08PM +0100, Théo Lebrun wrote:
> > > On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > > > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > > > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:

...

> > > > > > > +	priv->rcdev.of_node = np;
> > > > > >
> > > > > > It's better to use device_set_node().
> > > > > 
> > > > > I don't see how device_set_node() can help? It works on struct device
> > > > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > > > no users of device_set_node() in drivers/reset/.
> > > >
> > > > No users doesn't mean it's good. The API is relatively "new" and takes
> > > > care of two things:
> > > > 1) it uses agnostic interface;
> > > > 2) it doesn't require any firmware node direct dereference.
> > > >
> > > > The 2) is most important here as allows us to refactor (firmware node) code
> > > > in the future.
> > > 
> > > I think I get the point of device_set_node(). I still do not understand
> > > how it could help me fill the ->of_node field in a reset_controller_dev
> > > structure?
> >
> > Exactly why I put the above comment as recommendation. And then I elaborated
> > that entire reset framework should rather move towards fwnode.
> 
> OK now I get it. One question: would using fwnode abstractions make
> sense for a driver that is devicetree-only, and will stay that way?

In my opinion, yes. But less beneficial from it.

> However this sounds out-of-scope of such a driver addition. I also am
> not familiar enough (yet?) with the reset subsystem and/or fwnode to be
> able to bring this kind of changes upstream.

Right.
Andy Shevchenko Feb. 29, 2024, 3:36 p.m. UTC | #10
On Thu, Feb 29, 2024 at 04:28:42PM +0100, Philipp Zabel wrote:
> On Do, 2024-02-29 at 15:48 +0200, Andy Shevchenko wrote:
> > On Thu, Feb 29, 2024 at 01:18:08PM +0100, Théo Lebrun wrote:
> > > On Thu Feb 29, 2024 at 12:22 PM CET, Andy Shevchenko wrote:
> > > > On Wed, Feb 28, 2024 at 06:04:47PM +0100, Théo Lebrun wrote:
> > > > > On Tue Feb 27, 2024 at 6:27 PM CET, Andy Shevchenko wrote:
> > > > > > On Tue, Feb 27, 2024 at 03:55:25PM +0100, Théo Lebrun wrote:

...

> > > > > > > +	priv->rcdev.of_node = np;
> > > > > > 
> > > > > > It's better to use device_set_node().
> > > > > 
> > > > > I don't see how device_set_node() can help? It works on struct device
> > > > > pointers. Here priv->rcdev is a reset_controller_dev struct. There are
> > > > > no users of device_set_node() in drivers/reset/.
> > > > 
> > > > No users doesn't mean it's good. The API is relatively "new" and takes
> > > > care of two things:
> > > > 1) it uses agnostic interface;
> > > > 2) it doesn't require any firmware node direct dereference.
> > > > 
> > > > The 2) is most important here as allows us to refactor (firmware node) code
> > > > in the future.
> > > 
> > > I think I get the point of device_set_node(). I still do not understand
> > > how it could help me fill the ->of_node field in a reset_controller_dev
> > > structure?
> > 
> > Exactly why I put the above comment as recommendation. And then I elaborated
> > that entire reset framework should rather move towards fwnode.
> 
> For context, there have been initial patches for this, that turned out
> not to be necessary later on:
> 
> https://lore.kernel.org/lkml/20220324141237.297207-1-clement.leger@bootlin.com/
> 
> At this point, there still is no real use case for non-DT reset
> controls on the horizon.

I can argue on that if we have something like reset-gpio (and we have a such).

With this in place the ACPI can also provide descriptions for that.
Philipp Zabel March 1, 2024, 11:36 a.m. UTC | #11
On Do, 2024-02-29 at 17:36 +0200, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 04:28:42PM +0100, Philipp Zabel wrote:
> > On Do, 2024-02-29 at 15:48 +0200, Andy Shevchenko wrote:
> > > [...] And then I elaborated that entire reset framework should
> > > rather move towards fwnode.
> > 
> > For context, there have been initial patches for this, that turned out
> > not to be necessary later on:
> > 
> > https://lore.kernel.org/lkml/20220324141237.297207-1-clement.leger@bootlin.com/
> > 
> > At this point, there still is no real use case for non-DT reset
> > controls on the horizon.
> 
> I can argue on that if we have something like reset-gpio (and we have a such).

I've just sent out the pull request containing this, thank you for the
reminder.

> With this in place the ACPI can also provide descriptions for that.

Yes, an ACPI based device with shared GPIO resets (it is bound to
happen at some point...) would provide a reason to support ACPI GPIOs
in the reset framework.

regards
Philipp
diff mbox series

Patch

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index ccd59ddd7610..80bfde54c076 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -66,6 +66,18 @@  config RESET_BRCMSTB_RESCAL
 	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
 	  BCM7216.
 
+config RESET_EYEQ5
+	bool "Mobileye EyeQ5 reset controller"
+	depends on MFD_SYSCON
+	depends on MACH_EYEQ5 || COMPILE_TEST
+	default MACH_EYEQ5
+	help
+	  This enables the Mobileye EyeQ5 reset controller.
+
+	  It has three domains, with a varying number of resets in each of them.
+	  Registers are located in a shared register region called OLB accessed
+	  through a syscon & regmap.
+
 config RESET_HSDK
 	bool "Synopsys HSDK Reset Driver"
 	depends on HAS_IOMEM
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 8270da8a4baa..4fabe0070390 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -11,6 +11,7 @@  obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
 obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
 obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
+obj-$(CONFIG_RESET_EYEQ5) += reset-eyeq5.o
 obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
 obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
 obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c
new file mode 100644
index 000000000000..f9d3935dd420
--- /dev/null
+++ b/drivers/reset/reset-eyeq5.c
@@ -0,0 +1,342 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Reset driver for the Mobileye EyeQ5 platform.
+ *
+ * The registers are located in a syscon region called OLB. We handle three
+ * reset domains. Domains 0 and 2 look similar in that they both use one bit
+ * per reset line. Domain 1 has a register per reset.
+ *
+ * We busy-wait after updating a reset in domains 0 or 1. The reason is hardware
+ * logic built-in self-test (LBIST) that might be enabled.
+ *
+ * We use eq5r_ as prefix, as-in "EyeQ5 Reset", but way shorter.
+ *
+ * Known resets in domain 0:
+ *  3. CAN0
+ *  4. CAN1
+ *  5. CAN2
+ *  6. SPI0
+ *  7. SPI1
+ *  8. SPI2
+ *  9. SPI3
+ * 10. UART0
+ * 11. UART1
+ * 12. UART2
+ * 13. I2C0
+ * 14. I2C1
+ * 15. I2C2
+ * 16. I2C3
+ * 17. I2C4
+ * 18. TIMER0
+ * 19. TIMER1
+ * 20. TIMER2
+ * 21. TIMER3
+ * 22. TIMER4
+ * 23. WD0
+ * 24. EXT0
+ * 25. EXT1
+ * 26. GPIO
+ * 27. WD1
+ *
+ * Known resets in domain 1:
+ * 0. VMP0	(Vector Microcode Processors)
+ * 1. VMP1
+ * 2. VMP2
+ * 3. VMP3
+ * 4. PMA0	(Programmable Macro Array)
+ * 5. PMA1
+ * 6. PMAC0
+ * 7. PMAC1
+ * 8. MPC0	(Multi-threaded Processing Clusters)
+ * 9. MPC1
+ *
+ * Known resets in domain 2:
+ *  0. PCIE0_CORE
+ *  1. PCIE0_APB
+ *  2. PCIE0_LINK_AXI
+ *  3. PCIE0_LINK_MGMT
+ *  4. PCIE0_LINK_HOT
+ *  5. PCIE0_LINK_PIPE
+ *  6. PCIE1_CORE
+ *  7. PCIE1_APB
+ *  8. PCIE1_LINK_AXI
+ *  9. PCIE1_LINK_MGMT
+ * 10. PCIE1_LINK_HOT
+ * 11. PCIE1_LINK_PIPE
+ * 12. MULTIPHY
+ * 13. MULTIPHY_APB
+ * 15. PCIE0_LINK_MGMT
+ * 16. PCIE1_LINK_MGMT
+ * 17. PCIE0_LINK_PM
+ * 18. PCIE1_LINK_PM
+ *
+ * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
+ */
+
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+
+/* Domain 0 register offsets */
+#define D0_SARCR0	(0x004)
+#define D0_SARCR1	(0x008)
+
+/* Domain 1 masks */
+#define D1_ACRP_PD_REQ		BIT(0)
+#define D1_ACRP_ST_POWER_DOWN	BIT(27)
+#define D1_ACRP_ST_ACTIVE	BIT(29)
+
+/* Vendor-provided timeout values. D1 has a long timeout because of LBIST. */
+#define D0_TIMEOUT_POLL			10
+#define D1_TIMEOUT_POLL			40000
+
+/*
+ * Masks for valid reset lines in each domain. This array is also used to get
+ * the domain and reset counts.
+ */
+static const u32 eq5r_valid_masks[] = { 0x0FFFFFF8, 0x00001FFF, 0x0007BFFF };
+
+#define EQ5R_DOMAIN_COUNT ARRAY_SIZE(eq5r_valid_masks)
+
+struct eq5r_private {
+	struct mutex mutexes[EQ5R_DOMAIN_COUNT];
+	void __iomem *base_d0;
+	void __iomem *base_d1;
+	void __iomem *base_d2;
+	struct reset_controller_dev rcdev;
+};
+
+#define rcdev_to_priv(rcdev) container_of(rcdev, struct eq5r_private, rcdev)
+
+static int eq5r_busy_wait_withlock(struct eq5r_private *priv,
+				   struct device *dev, u32 domain, u32 offset,
+				   bool assert)
+{
+	unsigned int val, mask;
+	int i;
+
+	lockdep_assert_held(&priv->mutexes[domain]);
+
+	switch (domain) {
+	case 0:
+		for (i = 0; i < D0_TIMEOUT_POLL; i++) {
+			val = readl(priv->base_d0 + D0_SARCR1);
+			val = !(val & BIT(offset));
+			if (val == assert)
+				return 0;
+			udelay(1);
+		}
+		break;
+	case 1:
+		mask = assert ? D1_ACRP_ST_POWER_DOWN : D1_ACRP_ST_ACTIVE;
+		for (i = 0; i < D1_TIMEOUT_POLL; i++) {
+			val = readl(priv->base_d1 + 4 * offset);
+			if (val & mask)
+				return 0;
+			udelay(1);
+		}
+		break;
+	case 2:
+		return 0; /* No busy waiting for domain 2. */
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "%u-%u: timeout\n", domain, offset);
+	return -ETIMEDOUT;
+}
+
+static void eq5r_assert_withlock(struct eq5r_private *priv, u32 domain,
+				 u32 offset)
+{
+	void __iomem *reg;
+
+	lockdep_assert_held(&priv->mutexes[domain]);
+
+	switch (domain) {
+	case 0:
+		reg = priv->base_d0 + D0_SARCR0;
+		writel(readl(reg) & ~BIT(offset), reg);
+		break;
+	case 1:
+		reg = priv->base_d1 + 4 * offset;
+		writel(readl(reg) | D1_ACRP_PD_REQ, reg);
+		break;
+	case 2:
+		reg = priv->base_d2;
+		writel(readl(reg) & ~BIT(offset), reg);
+		break;
+	default:
+		WARN_ON(1);
+	}
+}
+
+static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct eq5r_private *priv = rcdev_to_priv(rcdev);
+	u32 offset = id & GENMASK(7, 0);
+	u32 domain = id >> 8;
+
+	dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
+
+	guard(mutex)(&priv->mutexes[domain]);
+	eq5r_assert_withlock(priv, domain, offset);
+	return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, true);
+}
+
+static void eq5r_deassert_withlock(struct eq5r_private *priv, u32 domain,
+				   u32 offset)
+{
+	void __iomem *reg;
+
+	lockdep_assert_held(&priv->mutexes[domain]);
+
+	switch (domain) {
+	case 0:
+		reg = priv->base_d0 + D0_SARCR0;
+		writel(readl(reg) | BIT(offset), reg);
+		break;
+	case 1:
+		reg = priv->base_d1 + 4 * offset;
+		writel(readl(reg) & ~D1_ACRP_PD_REQ, reg);
+		break;
+	case 2:
+		reg = priv->base_d2;
+		writel(readl(reg) | BIT(offset), reg);
+		break;
+	default:
+		WARN_ON(1);
+	}
+}
+
+static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct eq5r_private *priv = rcdev_to_priv(rcdev);
+	u32 offset = id & GENMASK(7, 0);
+	u32 domain = id >> 8;
+
+	dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset);
+
+	guard(mutex)(&priv->mutexes[domain]);
+	eq5r_deassert_withlock(priv, domain, offset);
+	return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, false);
+}
+
+static int eq5r_status(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct eq5r_private *priv = rcdev_to_priv(rcdev);
+	u32 offset = id & GENMASK(7, 0);
+	u32 domain = id >> 8;
+	u32 val;
+
+	dev_dbg(rcdev->dev, "%u-%u: status request\n", domain, offset);
+
+	guard(mutex)(&priv->mutexes[domain]);
+
+	switch (domain) {
+	case 0:
+		val = readl(priv->base_d0 + D0_SARCR1);
+		return !(val & BIT(offset));
+	case 1:
+		val = readl(priv->base_d1 + 4 * offset);
+		return !(val & D1_ACRP_ST_ACTIVE);
+	case 2:
+		val = readl(priv->base_d2);
+		return !(val & BIT(offset));
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct reset_control_ops eq5r_ops = {
+	.assert	  = eq5r_assert,
+	.deassert = eq5r_deassert,
+	.status	  = eq5r_status,
+};
+
+static int eq5r_of_xlate(struct reset_controller_dev *rcdev,
+			 const struct of_phandle_args *reset_spec)
+{
+	u32 domain, offset;
+
+	if (WARN_ON(reset_spec->args_count != 2))
+		return -EINVAL;
+
+	domain = reset_spec->args[0];
+	offset = reset_spec->args[1];
+
+	if (domain >= EQ5R_DOMAIN_COUNT || offset > 31 ||
+	    !(eq5r_valid_masks[domain] & BIT(offset))) {
+		dev_err(rcdev->dev, "%u-%u: invalid reset\n", domain, offset);
+		return -EINVAL;
+	}
+
+	return (domain << 8) | offset;
+}
+
+static int eq5r_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct eq5r_private *priv;
+	int ret, i;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base_d0 = devm_platform_ioremap_resource_byname(pdev, "d0");
+	if (IS_ERR(priv->base_d0))
+		return PTR_ERR(priv->base_d0);
+
+	priv->base_d1 = devm_platform_ioremap_resource_byname(pdev, "d1");
+	if (IS_ERR(priv->base_d1))
+		return PTR_ERR(priv->base_d1);
+
+	priv->base_d2 = devm_platform_ioremap_resource_byname(pdev, "d2");
+	if (IS_ERR(priv->base_d2))
+		return PTR_ERR(priv->base_d2);
+
+	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
+		mutex_init(&priv->mutexes[i]);
+
+	priv->rcdev.ops = &eq5r_ops;
+	priv->rcdev.owner = THIS_MODULE;
+	priv->rcdev.dev = dev;
+	priv->rcdev.of_node = np;
+	priv->rcdev.of_reset_n_cells = 2;
+	priv->rcdev.of_xlate = eq5r_of_xlate;
+
+	priv->rcdev.nr_resets = 0;
+	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
+		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
+
+	ret = devm_reset_controller_register(dev, &priv->rcdev);
+	if (ret) {
+		dev_err(dev, "Failed registering reset controller: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id eq5r_match_table[] = {
+	{ .compatible = "mobileye,eyeq5-reset" },
+	{}
+};
+
+static struct platform_driver eq5r_driver = {
+	.probe = eq5r_probe,
+	.driver = {
+		.name = "eyeq5-reset",
+		.of_match_table = eq5r_match_table,
+	},
+};
+
+builtin_platform_driver(eq5r_driver);