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

[Krzysztof Kozlowski]

On 24/01/2024 17:41, Théo Lebrun wrote:
> Hello,
> 
> On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote:
>> 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.
> 
> I wouldn't know how to handle the two steps then. Two clocks and the clk
> provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER().

Right, if some clocks have to be early, CLK_OF_DECLARE_DRIVER needs
static ones. But your commit subject says it is a platform driver and
all other pieces of this code is rather incompatible with this approach.

Do not use CLK_OF_DECLARE_DRIVER for cases where you have dependencies
because it forces you to manually order initcalls, which is exactly what
we do not want.


> The rest is at platform device probe. Without a static, there are no
> way to pass the struct clk_hw_onecell_data from one to the other.
> 
> I've looked at all clock drivers that do CLK_OF_DECLARE_DRIVER() and
> register a platform driver.

Even though the code is correct, using arguments "other did it" will not
work. You want to say that you implement legacy, poor code because you
saw legacy, poor code?

> 
>  - The following use a static variable:
>    drivers/clk/axis/clk-artpec6.c
>    drivers/clk/clk-aspeed.c
>    drivers/clk/clk-ast2600.c
>    drivers/clk/clk-eyeq5.c
>    drivers/clk/clk-gemini.c
>    drivers/clk/clk-milbeaut.c
>    drivers/clk/mediatek/clk-mt2701.c
>    drivers/clk/mediatek/clk-mt6797.c
>    drivers/clk/mediatek/clk-mt8173-infracfg.c
>    drivers/clk/nxp/clk-lpc18xx-creg.c
>    drivers/clk/ralink/clk-mt7621.c
>    drivers/clk/ralink/clk-mtmips.c
>    drivers/clk/sunxi/clk-mod0.c
>    drivers/clk/axis/clk-artpec6.c
> 
>  - Those two declare different clock providers at init and probe:
>    drivers/clk/ralink/clk-mt7621.c
>    drivers/clk/sunxi/clk-mod0.c
> 
>  - It doesn't register new clocks at probe (only resets) so no need to
>    share variables.
>    drivers/clk/ralink/clk-mtmips.c
> 
>>
>> ...
>>
>>> +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.
> 
> of_syscon_register() and regmap_init_mmio() lists many reasons for
> it to not be available. Am I missing something?

Yes, initcall ordering.

Best regards,
Krzysztof