| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 27 Feb 2024 19:27:08 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Théo Lebrun <theo.lebrun@...tlin.com>
Cc: Gregory CLEMENT <gregory.clement@...tlin.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Linus Walleij <linus.walleij@...aro.org>,
Rafał Miłecki <rafal@...ecki.pl>,
Philipp Zabel <p.zabel@...gutronix.de>,
Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
linux-mips@...r.kernel.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Tawfik Bayouk <tawfik.bayouk@...ileye.com>,
linux-gpio@...r.kernel.org
Subject: Re: [PATCH v8 04/10] reset: eyeq5: add platform driver
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);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists