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