| 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
| ||
|
Message-Id: <D0H8EDEDKGV9.2FT5JGT59AU8A@bootlin.com>
Date: Thu, 11 Apr 2024 12:46:04 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Stephen Boyd" <sboyd@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>, "Linus Walleij"
<linus.walleij@...aro.org>, "Michael Turquette" <mturquette@...libre.com>,
"Philipp Zabel" <p.zabel@...gutronix.de>, "Rob Herring" <robh@...nel.org>
Cc: <linux-mips@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-clk@...r.kernel.org>,
<linux-gpio@...r.kernel.org>, "Vladimir Kondratiev"
<vladimir.kondratiev@...ileye.com>, "Gregory CLEMENT"
<gregory.clement@...tlin.com>, "Thomas Petazzoni"
<thomas.petazzoni@...tlin.com>, "Tawfik Bayouk"
<tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH 05/11] clk: eyeq: add driver
Hello,
On Thu Apr 11, 2024 at 5:22 AM CEST, Stephen Boyd wrote:
> Quoting Théo Lebrun (2024-04-10 10:12:34)
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 50af5fc7f570..1eb6e70977a3 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -218,6 +218,17 @@ config COMMON_CLK_EN7523
> > This driver provides the fixed clocks and gates present on Airoha
> > ARM silicon.
> >
> > +config COMMON_CLK_EYEQ
> > + bool "Clock driver for the Mobileye EyeQ platform"
> > + depends on OF || COMPILE_TEST
>
> The OF build dependency looks useless as we have the MACH_ dependency
> below.
Indeed. I thought explicit dependency could be useful. Will remove.
> > + depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST
> > + default MACH_EYEQ5 || MACH_EYEQ6H
> > + help
> > + This driver provides clocks found on Mobileye EyeQ5, EyeQ6L and Eye6H
> > + SoCs. Controllers live in shared register regions called OLB. Driver
> > + provides read-only PLLs, derived from the main crystal clock (which
> > + must be constant). It also exposes some divider clocks.
> > +
> > config COMMON_CLK_FSL_FLEXSPI
> > tristate "Clock driver for FlexSPI on Layerscape SoCs"
> > depends on ARCH_LAYERSCAPE || COMPILE_TEST
> > diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> > new file mode 100644
> > index 000000000000..bb2535010ae6
> > --- /dev/null
> > +++ b/drivers/clk/clk-eyeq.c
> > @@ -0,0 +1,644 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * PLL clock driver for the Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms.
> > + *
> > + * This controller handles read-only PLLs, all derived from the same main
> > + * crystal clock. It also exposes divider clocks, those are children to PLLs.
> > + * Parent clock is expected to be constant. This driver's registers live in
> > + * a shared region called OLB. Some PLLs are initialised early by of_clk_init().
>
> Is OLB a different DT node? It sounds like maybe this is trying to jam a
> driver into DT when the OLB node should be a #clock-cells node.
Yes OLB is a different DT node. It looks like on EyeQ5:
olb: system-controller@...000 {
compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
reg = <0 0xe00000 0x0 0x400>;
ranges = <0x0 0x0 0xe00000 0x400>;
#address-cells = <1>;
#size-cells = <1>;
reset: reset-controller@...000 {
compatible = "mobileye,eyeq5-reset";
reg = <0x000 0x0c>, <0x200 0x34>, <0x120 0x04>;
reg-names = "d0", "d1", "d2";
#reset-cells = <2>;
};
clocks: clock-controller@...02c {
compatible = "mobileye,eyeq5-clk";
reg = <0x02c 0x50>, <0x11c 0x04>;
reg-names = "plls", "ospi";
#clock-cells = <1>;
clocks = <&xtal>;
clock-names = "ref";
};
pinctrl: pinctrl@...0b0 {
compatible = "mobileye,eyeq5-pinctrl";
reg = <0x0b0 0x30>;
};
};
Keep in mind OLB is a complex beast. On EyeQ5, it hosts something like
150 registers, describing 20ish various hardware features. We have to
expose registers to drivers for one-off reads/writes. One example found
upstream: I2C speed mode register. Others will be Ethernet, eMMC DMA
config, etc. A syscon makes sense.
I2C looks like like this for example, look at mobileye,olb.
i2c@...000 {
compatible = "mobileye,eyeq5-i2c", "arm,primecell";
reg = <0x300000 0x1000>;
interrupt-parent = <&gic>;
interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
clock-frequency = <400000>;
#address-cells = <1>;
#size-cells = <0>;
clocks = <&i2c_ser_clk>, <&i2c_clk>;
clock-names = "i2cclk", "apb_pclk";
mobileye,olb = <&olb 0>;
};
See commits 7d4c57abb928 and 1b9a8e8af0d9:
i2c: nomadik: support Mobileye EyeQ5 I2C controller
dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
> > +
> > +static int eqc_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + void __iomem *div_resources[EQC_MAX_DIV_COUNT];
> > + struct device_node *np = dev->of_node;
> > + const struct eqc_match_data *data;
> > + struct eqc_priv *priv = NULL;
> > + struct clk_hw *hw;
> > + unsigned int i;
> > +
> > + data = device_get_match_data(dev);
> > + if (!data)
> > + return -ENODEV;
> > +
> > + if (data->early_pll_count) {
> > + /* Device got inited early. Retrieve clock provider from list. */
> > + struct eqc_priv *entry;
> > +
> > + spin_lock(&eqc_list_slock);
> > + list_for_each_entry(entry, &eqc_list, list) {
> > + if (entry->np == np) {
> > + priv = entry;
> > + break;
> > + }
> > + }
> > + spin_unlock(&eqc_list_slock);
> > +
> > + if (!priv)
> > + return -ENODEV;
>
> This can be a sub-function.
Will do.
[...]
> > + for (i = 0; i < data->pll_count; i++) {
> > + const struct eqc_pll *pll = &data->plls[i];
> > + unsigned long mult, div, acc;
> > + u32 r0, r1;
> > + u64 val;
> > + int ret;
>
> All variables should be declared at the start of the function. Once it
> becomes "too heavy" you can split it up into smaller functions, that
> again have all variables declared at the start of the function.
Will avoid variables declarations at start of loops.
> > +
> > + val = readq(priv->base_plls + pll->reg64);
> > + r0 = val;
> > + r1 = val >> 32;
> > +
> > + ret = eqc_pll_parse_registers(r0, r1, &mult, &div, &acc);
> > + if (ret) {
> > + dev_warn(dev, "failed parsing state of %s\n", pll->name);
> > + priv->cells->hws[pll->index] = ERR_PTR(ret);
> > + continue;
> > + }
> > +
> > + hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev,
> > + dev->of_node, pll->name, "ref", 0, mult, div, acc);
> > + priv->cells->hws[pll->index] = hw;
> > + if (IS_ERR(hw))
> > + dev_warn(dev, "failed registering %s: %pe\n", pll->name, hw);
> > + }
> > +
> > + BUG_ON(ARRAY_SIZE(div_resources) < data->div_count);
>
> Can this be a static assert instead on the arrays these are based on?
> Put some static_assert() near the match data macros.
I hesitated before sending. Will update.
> > +
> > + for (i = 0; i < data->div_count; i++) {
> > + const struct eqc_div *div = &data->divs[i];
> > + void __iomem *base = NULL;
> > + struct clk_hw *parent;
> > + unsigned int j;
> > +
> > + /*
> > + * Multiple divider clocks can request the same resource. Store
> > + * resource pointers during probe(). For each divider clock,
> > + * check if previous clocks referenced the same resource name.
> > + *
> > + * See EQ6HC_SOUTH_DIV_OSPI_REF and EQ6HC_SOUTH_DIV_OSPI_SYS.
> > + */
> > + for (j = 0; j < i; j++) {
> > + if (strcmp(data->divs[j].resource_name, div->resource_name) == 0) {
> > + base = div_resources[j];
> > + break;
> > + }
> > + }
> > +
> > + /* Resource is first encountered. */
> > + if (!base) {
> > + base = devm_platform_ioremap_resource_byname(pdev, div->resource_name);
> > + if (IS_ERR(base)) {
> > + dev_warn(dev, "failed to iomap resource for %s\n", div->name);
> > + priv->cells->hws[div->index] = base;
> > + continue;
> > + }
> > + }
>
> I don't get this code at all. The driver should simply map the
> resources because it knows that there's an io resource. I'll look at the
> binding which is probably wrong and causing the driver to be written
> this way.
This is here for a single reason: EyeQ6H south OLB has two clocks that
live in the same register:
- div-ospi-ref, reg offset 0x90, mask GENMASK(9, 8) == 0x300.
- div-ospi-sys, reg offset 0x90, mask GENMASK(12, 4) == 0x1FF0.
Calling twice devm_platform_ioremap_resource_byname() with the same
resource name gives an error. So we need to buffer resources already
requested.
If there is a simpler & better solution I'd be happy to take it.
[...]
> > +static void __init eqc_init(struct device_node *np)
> > +{
> > + const struct eqc_match_data *data;
> > + unsigned int nb_clks = 0;
> > + struct eqc_priv *priv;
> > + unsigned int i;
> > + int ret;
> > +
> > + data = of_match_node(eqc_match_table, np)->data;
> > +
> > + /* No reason to early init this clock provider. Do it at probe. */
> > + if (data->early_pll_count == 0)
>
> You can have a different match table for this function then.
Ah, clever. Will do.
[...]
> > + /*
> > + * We expect named resources if divider clocks are present.
> > + * Else, we only expect one resource.
> > + */
>
> Please avoid named resources. They give the false sense of hope that the
> binding can re-order the reg property when that can't be done. Instead,
> just index and know which index to use in the driver.
It is unclear what you mean by not being able to re-order reg property?
Are you talking about reg-names being most often defined as items const
list and therefore cannot be reordered? Here binding declare things
using minItems/maxItems/enum so it can be reordered, looking like:
properties:
reg:
minItems: 2
maxItems: 2
reg-names:
minItems: 2
maxItems: 2
items:
enum: [ plls, ospi ]
If this is not what you are talking about then I rambled about garbage
and I'll use indexed resources.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists