Hello Philipp,
On Mon Jul 1, 2024 at 10:59 AM CEST, Philipp Zabel wrote:
> On Fr, 2024-06-28 at 18:11 +0200, Théo Lebrun wrote:
> > Add Mobileye EyeQ reset controller driver, for EyeQ5, EyeQ6L and EyeQ6H
> > SoCs. Instances belong to a shared register region called OLB and gets
> > spawned as auxiliary device to the platform driver for clock.
> >
> > There is one OLB instance for EyeQ5 and EyeQ6L. There are seven OLB
> > instances on EyeQ6H; three have a reset controller embedded:
> > - West and east get handled by the same compatible.
> > - Acc (accelerator) is another one.
> >
> > Each instance vary in the number and types of reset domains.
> > Instances with single domain expect a single cell, others two.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> > ---
> > drivers/reset/Kconfig | 13 ++
> > drivers/reset/Makefile | 1 +
> > drivers/reset/reset-eyeq.c | 562 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 576 insertions(+)
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 7112f5932609..14f3f4af0b10 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -66,6 +66,19 @@ config RESET_BRCMSTB_RESCAL
> > This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
> > BCM7216.
> >
> > +config RESET_EYEQ
> > + bool "Mobileye EyeQ reset controller"
> > + depends on AUXILIARY_BUS
>
> This should
>
> select AUXILIARY_BUS
>
> instead.
Done, looking like this now:
config RESET_EYEQ
bool "Mobileye EyeQ reset controller"
depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST
select AUXILIARY_BUS
default MACH_EYEQ5 || MACH_EYEQ6H
help
...
[...]
> > +#define EQR_MAX_DOMAIN_COUNT 3
> > +
> > +struct eqr_domain_descriptor {
> > + enum eqr_domain_type type;
> > + u32 valid_mask;
> > + unsigned int offset;
> > +};
> > +
> > +struct eqr_match_data {
> > + unsigned int domain_count;
> > + const struct eqr_domain_descriptor *domains;
> > +};
> > +
> > +struct eqr_private {
> > + struct mutex mutexes[EQR_MAX_DOMAIN_COUNT];
>
> Is there any benefit from per-domain mutexes over just a single mutex?
Some domains can stay locked for pretty long in theory because of Logic
built-in self-test (LBIST). This is the reasoning behind
eqr_busy_wait_locked().
Other domains are not concerned by this behavior.
More concretely, on EyeQ5, SARCR and ACRP domains can lock their mutexes
for a relatively long time, and for good reasons. We wouldn't want the
PCIE domain to suffer from that if it happens to (de)assert a reset at
the same time.
>
> > + void __iomem *base;
> > + const struct eqr_match_data *data;
> > + struct reset_controller_dev rcdev;
> > +};
> > +
> > +#define rcdev_to_priv(rcdev) container_of(rcdev, struct eqr_private, rcdev)
>
> Please use checkpatch --strict, and ideally mention when you ignore a
> warning on purpose. In this case, the macro parameter should named
> something else, because the last parameter to container_of must be
> "rcdev" verbatim. This only works by accident because the passed
> parameter also happens to be called called "rcdev" at all call sites.
I have let this CHECK from `checkpatch --strict` slip through indeed.
Other remaining messages, with explanations, are:
- WARNING: added, moved or deleted file(s), does MAINTAINERS need
updating?
This is done in a single patch [0] in the MIPS series to avoid
conflicts in between series.
- CHECK: struct mutex definition without comment
This is about the above mutexes field. Do you want a code comment
about the reasoning for one mutex per domain?
- WARNING: DT compatible string "[...]" appears un-documented
Bindings are added in a single commit in the MIPS series [1],
to avoid conflicts.
>
> > +static u32 eqr_double_readl(void __iomem *addr_a, void __iomem *addr_b,
> > + u32 *dest_a, u32 *dest_b)
> > +{
> > + *dest_a = readl(addr_a);
> > + *dest_b = readl(addr_b);
> > + return 0; /* read_poll_timeout() op argument must return something. */
> > +}
> > +
> > +static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev,
> > + u32 domain, u32 offset, bool assert)
> > +{
> > + enum eqr_domain_type domain_type = priv->data->domains[domain].type;
> > + unsigned long sleep_us, timeout_us;
> > + u32 val, mask, val0, val1;
> > + void __iomem *base, *reg;
> > + int ret;
> > +
> > + lockdep_assert_held(&priv->mutexes[domain]);
> > +
> > + base = priv->base + priv->data->domains[domain].offset;
> > + sleep_us = eqr_timings[domain_type].sleep_us;
> > + timeout_us = eqr_timings[domain_type].timeout_us;
>
> You can initialize these at the declaration.
Done, declarations will look like:
static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev,
u32 domain, u32 offset, bool assert)
{
void __iomem *base = priv->base + priv->data->domains[domain].offset;
enum eqr_domain_type domain_type = priv->data->domains[domain].type;
unsigned long timeout_us = eqr_timings[domain_type].timeout_us;
unsigned long sleep_us = eqr_timings[domain_type].sleep_us;
u32 val, mask, val0, val1;
void __iomem *reg;
int ret;
// ...
}
>
> > +
> > + switch (domain_type) {
> > + case EQR_EYEQ5_SARCR:
> > + reg = base + EQR_EYEQ5_SARCR_STATUS;
> > + mask = BIT(offset);
> > +
> > + ret = readl_poll_timeout(reg, val, !(val & mask) == assert,
> > + sleep_us, timeout_us);
> > + break;
> > +
> > + case EQR_EYEQ5_ACRP:
> > + reg = base + 4 * offset;
> > + if (assert)
> > + mask = EQR_EYEQ5_ACRP_ST_POWER_DOWN;
> > + else
> > + mask = EQR_EYEQ5_ACRP_ST_ACTIVE;
> > +
> > + ret = readl_poll_timeout(reg, val, !!(val & mask),
> > + sleep_us, timeout_us);
> > + break;
> > +
> > + case EQR_EYEQ5_PCIE:
> > + ret = 0; /* No busy waiting. */
> > + break;
> > +
> > + case EQR_EYEQ6H_SARCR:
> > + /*
> > + * Wait until both bits change:
> > + * readl(base + EQR_EYEQ6H_SARCR_RST_STATUS) & BIT(offset)
> > + * readl(base + EQR_EYEQ6H_SARCR_CLK_STATUS) & BIT(offset)
> > + */
> > + mask = BIT(offset);
> > + ret = read_poll_timeout(eqr_double_readl, val,
> > + (!(val0 & mask) == assert) &&
> > + (!(val1 & mask) == assert),
>
> I'd remove a level of indentation here.
Indeed!
> > + sleep_us, timeout_us, false,
> > + base + EQR_EYEQ6H_SARCR_RST_STATUS,
> > + base + EQR_EYEQ6H_SARCR_CLK_STATUS,
> > + &val0, &val1);
>
> Calling these variables something like rst_status and clk_status, would
> make this a bit easier to parse.
It now looks like:
static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev,
u32 domain, u32 offset, bool assert)
{
// ...
switch (domain_type) {
// ...
case EQR_EYEQ6H_SARCR:
/*
* Wait until both bits change:
* readl(base + EQR_EYEQ6H_SARCR_RST_STATUS) & BIT(offset)
* readl(base + EQR_EYEQ6H_SARCR_CLK_STATUS) & BIT(offset)
*/
mask = BIT(offset);
ret = read_poll_timeout(eqr_double_readl, val,
(!(rst_status & mask) == assert) &&
(!(clk_status & mask) == assert),
sleep_us, timeout_us, false,
base + EQR_EYEQ6H_SARCR_RST_STATUS,
base + EQR_EYEQ6H_SARCR_CLK_STATUS,
&rst_status, &clk_status);
break;
// ...
}
// ...
}
>
> > + break;
> > +
> > + default:
> > + WARN_ON(1);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + if (ret == -ETIMEDOUT)
> > + dev_dbg(dev, "%u-%u: timeout\n", domain, offset);
> > + return ret;
> > +}
> > +
> > +static void eqr_assert_locked(struct eqr_private *priv, u32 domain, u32 offset)
> > +{
> > + enum eqr_domain_type domain_type = priv->data->domains[domain].type;
> > + void __iomem *base, *reg;
> > + u32 val;
> > +
> > + lockdep_assert_held(&priv->mutexes[domain]);
> > +
> > + base = priv->base + priv->data->domains[domain].offset;
> > +
> > + switch (domain_type) {
> > + case EQR_EYEQ5_SARCR:
> > + reg = base + EQR_EYEQ5_SARCR_REQUEST;
> > + writel(readl(reg) & ~BIT(offset), reg);
> > + break;
> > +
> > + case EQR_EYEQ5_ACRP:
> > + reg = base + 4 * offset;
> > + writel(readl(reg) | EQR_EYEQ5_ACRP_PD_REQ, reg);
> > + break;
> > +
> > + case EQR_EYEQ5_PCIE:
> > + writel(readl(base) & ~BIT(offset), base);
> > + break;
> > +
> > + case EQR_EYEQ6H_SARCR:
> > + val = readl(base + EQR_EYEQ6H_SARCR_RST_REQUEST);
> > + val &= ~BIT(offset);
> > + writel(val, base + EQR_EYEQ6H_SARCR_RST_REQUEST);
> > + writel(val, base + EQR_EYEQ6H_SARCR_CLK_REQUEST);
>
> This looks peculiar. Why is it ok to write the value read from
> RST_REQUEST into CLK_REQUEST?
What is abstracted away by the hardware on EyeQ5 is not anymore on
EyeQ6H. Previously a single register was used for requests and a single
register for status. Now there are two request registers and two status
registers.
Those registers *must be kept in sync*. The register name referencing
clock is not to be confused with the clock driver of the
system-controller. It is describing a register within the reset
controller.
This hardware interface is odd, I might add a comment?
[...]
> > +static int eqr_status(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > + u32 domain = FIELD_GET(ID_DOMAIN_MASK, id);
> > + struct eqr_private *priv = rcdev_to_priv(rcdev);
> > + enum eqr_domain_type domain_type = priv->data->domains[domain].type;
> > + u32 offset = FIELD_GET(ID_OFFSET_MASK, id);
>
> I'd put domain and offset declaration next to each other.
Done:
static int eqr_status(struct reset_controller_dev *rcdev, unsigned long id)
{
u32 domain = FIELD_GET(ID_DOMAIN_MASK, id);
u32 offset = FIELD_GET(ID_OFFSET_MASK, id);
struct eqr_private *priv = rcdev_to_priv(rcdev);
enum eqr_domain_type domain_type = priv->data->domains[domain].type;
void __iomem *base, *reg;
// ...
}
I'll be sending a new revision in the week with those fixes.
Thanks Philipp,
[0]: https://lore.kernel.org/lkml/20240628-mbly-mips-v1-3-f53f5e4c422b@xxxxxxxxxxx/
[1]: https://lore.kernel.org/lkml/20240628-mbly-mips-v1-1-f53f5e4c422b@xxxxxxxxxxx/
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
