linux-kernel - Re: [PATCH v3 08/17] clk: eyeq5: add platform driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <127fd51b-cd64-4e00-99d6-7be9b79f2dcc@linaro.org>
Date: Wed, 24 Jan 2024 08:05:37 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
 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>
Cc: 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 v3 08/17] clk: eyeq5: add platform driver

On 23/01/2024 19:46, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> support for other platforms from Mobileye.
> 
> It handles 10 read-only PLLs derived from the main crystal on board. It
> exposes a table-based divider clock used for OSPI. Other platform
> clocks are not configurable and therefore kept as fixed-factor
> devicetree nodes.
> 
> Two PLLs are required early on and are therefore registered at
> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
> UARTs.
> 


> +#define OLB_PCSR1_RESET				BIT(0)
> +#define OLB_PCSR1_SSGC_DIV			GENMASK(4, 1)
> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
> +#define OLB_PCSR1_SPREAD			GENMASK(9, 5)
> +#define OLB_PCSR1_DIS_SSCG			BIT(10)
> +/* Down-spread or center-spread */
> +#define OLB_PCSR1_DOWN_SPREAD			BIT(11)
> +#define OLB_PCSR1_FRAC_IN			GENMASK(31, 12)
> +
> +static struct clk_hw_onecell_data *eq5c_clk_data;
> +static struct regmap *eq5c_olb;

Drop these two. No file-scope regmaps for drivers. Use private container
structures.

..

> +static void __init eq5c_init(struct device_node *np)
> +{
> +	struct device_node *parent_np = of_get_parent(np);
> +	int i, ret;
> +
> +	eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
> +				GFP_KERNEL);
> +	if (!eq5c_clk_data) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	eq5c_clk_data->num = EQ5C_NB_CLKS;
> +
> +	/*
> +	 * Mark all clocks as deferred. We register some now and others at
> +	 * platform device probe.
> +	 */
> +	for (i = 0; i < EQ5C_NB_CLKS; i++)
> +		eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> +
> +	/*
> +	 * Currently, if OLB is not available, we log an error, fail init then

How it could be not available? Only with broken initcall ordering. Fix
your initcall ordering and then simplify all this weird code.

> +	 * fail probe. We might want to change this behavior and assume all
> +	 * clocks are in bypass mode; this is what is being done in the vendor
> +	 * driver.
> +	 *
> +	 * It is unclear if there are valid situations where the OLB region
> +	 * would be inaccessible.



Best regards,
Krzysztof