Re: [PATCH 12/16] irqchip/eip201-aic: Add support for Safexcel EIP-201 AIC

Re: [PATCH 12/16] irqchip/eip201-aic: Add support for Safexcel EIP-201 AIC

[Thomas Gleixner]

On Fri, Mar 27 2026 at 21:09, Miquel Raynal wrote:
> +config SAFEXCEL_EIP201_AIC
> +        tristate "Safexcel EIP201 AIC"

TAB, not spaces please

> +	select IRQ_DOMAIN
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2026 Schneider Electric
> + * Authored by Miquel Raynal <miquel.raynal@bootlin.com>
> + * Based on the work from Mathieu Hadjimegrian <mathieu.hadjimegrian@non.se.com>
> + */
> +
> +#include "linux/irq.h"
> +#include "linux/stddef.h"

That's not a standard include format.

> +
> +struct eip201_aic {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct irq_domain *domain;
> +	struct irq_chip_generic *gc;
> +	u32 type;
> +	u32 pol;
> +};

Please follow:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

> +
> +	/* Type register indicates:

See 'comment style' in the same document.

> +	 * - '1' for edge interrupts
> +	 * - '0' for level interrupts
> +	 */
> +	if (*out_type & IRQ_TYPE_LEVEL_MASK &&
> +	    EIP201_AIC_INT(aic->type, *out_hwirq))

No line break required. You have 100 characters.

> +static int eip201_aic_probe(struct platform_device *pdev)
> +{
> +	struct eip201_aic *aic;
> +	struct clk *clk;
> +	u32 rev;
> +	int irq;
> +	int ret;

See 'variable declarations' in the same document.

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;

Leaks the chip and the domain.

> +static struct platform_driver eip201_aic_driver = {
> +	.probe = eip201_aic_probe,
> +	.remove = eip201_aic_remove,
> +	.driver = {
> +		.name = "safexcel-eip201-aic",
> +		.of_match_table = eip201_aic_of_match,

See above.

Thanks,

        tglx

Re: [PATCH 12/16] irqchip/eip201-aic: Add support for Safexcel EIP-201 AIC

[Miquel Raynal]

Hello Thomas,

>> +struct eip201_aic {
>> +	struct device *dev;
>> +	void __iomem *regs;
>> +	struct irq_domain *domain;
>> +	struct irq_chip_generic *gc;
>> +	u32 type;
>> +	u32 pol;
>> +};
>
> Please follow:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Ah, I didn't know about this document, I'll go through it and fix the style.

Thanks for the feedback,
Miquèl

Re: [PATCH 00/16] Add support for Inside-Secure EIP-150 crypto block

[Geert Uytterhoeven]

Hi Miquel,

On Fri, 27 Mar 2026 at 21:10, Miquel Raynal (Schneider Electric)
<miquel.raynal@bootlin.com> wrote:
> This is a series adding support for the EIP-150, which is a crypto block
> containing:
> - a public key accelerator
> - a random number generator
> - an interrupt controller

Thanks for your series!

>       irqchip/eip201-aic: Add support for Safexcel EIP-201 AIC
[...]
>       crypto: eip28: Add support for SafeXcel EIP-28 Public Key Accelerator

My OCD tells me to ask for using "SafeXcel" consistently,  ;-)

drivers/crypto/inside-secure/eip28.c: .name = "Safexcel EIP28 PKA",
drivers/irqchip/Kconfig:        tristate "Safexcel EIP201 AIC"
drivers/irqchip/Kconfig:   inside Safexcel EIP150 IPs, gathering
Public Key Accelerator

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 00/16] Add support for Inside-Secure EIP-150 crypto block

[Miquel Raynal]

Hi Geert,

On 30/03/2026 at 15:33:30 +02, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Miquel,
>
> On Fri, 27 Mar 2026 at 21:10, Miquel Raynal (Schneider Electric)
> <miquel.raynal@bootlin.com> wrote:
>> This is a series adding support for the EIP-150, which is a crypto block
>> containing:
>> - a public key accelerator
>> - a random number generator
>> - an interrupt controller
>
> Thanks for your series!
>
>>       irqchip/eip201-aic: Add support for Safexcel EIP-201 AIC
> [...]
>>       crypto: eip28: Add support for SafeXcel EIP-28 Public Key Accelerator
>
> My OCD tells me to ask for using "SafeXcel" consistently,  ;-)

Ah, yeah :) I initially wrote "Safexcel" everywhere, and at some point I
realized the marketing department had put a capital letter in the middle
of the word. My anti kamel-case heart fought back, but not enough, ending
up with a mix of both.

> drivers/crypto/inside-secure/eip28.c: .name = "Safexcel EIP28 PKA",
> drivers/irqchip/Kconfig:        tristate "Safexcel EIP201 AIC"
> drivers/irqchip/Kconfig:   inside Safexcel EIP150 IPs, gathering
> Public Key Accelerator

I'll address these.

Thanks,
Miquèl

Re: [PATCH 06/16] clk: tests: Add clk_parse_clkspec() Kunit testing

[Brian Masney]

Hi Miquel,

On Fri, Mar 27, 2026 at 09:09:28PM +0100, Miquel Raynal (Schneider Electric) wrote:
> Create a new set of kunit tests to make sure clk_parse_clkspec() is
> working as expected. We currently verify if we get a proper device when
> using indexes and names. If we make an out of bounds request we expect
> an error.
> 
> For testing purposes, we must ensure of_clk_get_hw()'s symbol is
> exported.
> 
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  drivers/clk/Makefile                     |   1 +
>  drivers/clk/clk.c                        |   1 +
>  drivers/clk/clk_test.c                   | 124 +++++++++++++++++++++++++++++++
>  drivers/clk/kunit_clk_parse_clkspec.dtso |  21 ++++++
>  4 files changed, 147 insertions(+)
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index f7bce3951a30..97b621456bf5 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -19,6 +19,7 @@ clk-test-y			:= clk_test.o \
>  				   kunit_clk_assigned_rates_zero.dtbo.o \
>  				   kunit_clk_assigned_rates_zero_consumer.dtbo.o \
>  				   kunit_clk_hw_get_dev_of_node.dtbo.o \
> +				   kunit_clk_parse_clkspec.dtbo.o \
>  				   kunit_clk_parent_data_test.dtbo.o
>  obj-$(CONFIG_COMMON_CLK)	+= clk-divider.o
>  obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 47093cda9df3..1795246b10a0 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5312,6 +5312,7 @@ struct clk_hw *of_clk_get_hw(struct device_node *np, int index,
>  
>  	return hw;
>  }
> +EXPORT_SYMBOL_GPL(of_clk_get_hw);

So that we don't unnecessarily broaden the API that's available to the
clk providers, you can use EXPORT_SYMBOL_IF_KUNIT so that this is only
available to the kunit tests.

Note that Chen-Yu posted a separate patch to add the includes for a
separate test. The two patches will conflict since Stephen hasn't picked
this up yet.

https://lore.kernel.org/linux-clk/20260225083413.3384950-1-wenst@chromium.org/

>  
>  static struct clk *__of_clk_get(struct device_node *np,
>  				int index, const char *dev_id,
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index a268d7b5d4cb..b814b45f1f7e 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -3541,10 +3541,134 @@ static struct kunit_suite clk_hw_get_dev_of_node_test_suite = {
>  	.test_cases = clk_hw_get_dev_of_node_test_cases,
>  };
>  
> +static const struct clk_init_data clk_parse_clkspec_1_init_data = {
> +	.name = "clk_parse_clkspec_1",
> +	.ops = &empty_clk_ops,
> +};
> +
> +static const struct clk_init_data clk_parse_clkspec_2_init_data = {
> +	.name = "clk_parse_clkspec_2",
> +	.ops = &empty_clk_ops,
> +};
> +
> +static struct clk_hw *kunit_clk_get(struct of_phandle_args *clkspec, void *data)
> +{
> +	return (struct clk_hw *)data;
> +}
> +
> +struct clk_parse_clkspec_ctx {
> +	struct device_node *prov1_np;
> +	struct device_node *prov2_np;
> +	struct device_node *cons_np;
> +};
> +
> +static int clk_parse_clkspec_init(struct kunit *test)
> +{
> +	struct clk_parse_clkspec_ctx *ctx;
> +	struct clk_hw *hw1, *hw2;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +	test->priv = ctx;
> +
> +	KUNIT_ASSERT_EQ(test, 0, of_overlay_apply_kunit(test, kunit_clk_parse_clkspec));
> +
> +	/* Register provider 1 */
> +	hw1 = kunit_kzalloc(test, sizeof(*hw1), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw1);
> +	hw1->init = &clk_parse_clkspec_1_init_data;
> +
> +	ctx->prov1_np = of_find_compatible_node(NULL, NULL, "test,clock-provider1");
> +	KUNIT_ASSERT_NOT_NULL(test, ctx->prov1_np);
> +
> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->prov1_np, hw1));
> +	of_clk_add_hw_provider(ctx->prov1_np, kunit_clk_get, hw1);

Can you just use of_clk_hw_simple_get() and drop kunit_clk_get() above?

> +	of_node_put(ctx->prov1_np);
> +
> +	/* Register provider 2 */
> +	hw2 = kunit_kzalloc(test, sizeof(*hw2), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw2);
> +	hw2->init = &clk_parse_clkspec_2_init_data;
> +
> +	ctx->prov2_np = of_find_compatible_node(NULL, NULL, "test,clock-provider2");
> +	KUNIT_ASSERT_NOT_NULL(test, ctx->prov2_np);
> +
> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->prov2_np, hw2));
> +	of_clk_add_hw_provider(ctx->prov2_np, kunit_clk_get, hw2);
> +	of_node_put(ctx->prov2_np);
> +
> +	ctx->cons_np = of_find_compatible_node(NULL, NULL, "test,clock-consumer");
> +	KUNIT_ASSERT_NOT_NULL(test, ctx->cons_np);
> +
> +	return 0;
> +}
> +
> +static void clk_parse_clkspec_exit(struct kunit *test)
> +{
> +	struct clk_parse_clkspec_ctx *ctx = test->priv;
> +
> +	of_node_put(ctx->prov1_np);
> +	of_node_put(ctx->prov2_np);

Is there a double free of prov1_np and prov2_np? If this is dropped from
the test exit, then they should't need to be in the ctx struct.

Brian

Re: [PATCH 06/16] clk: tests: Add clk_parse_clkspec() Kunit testing

[Miquel Raynal]

Hi Brian,

>> @@ -5312,6 +5312,7 @@ struct clk_hw *of_clk_get_hw(struct device_node *np, int index,
>>  
>>  	return hw;
>>  }
>> +EXPORT_SYMBOL_GPL(of_clk_get_hw);
>
> So that we don't unnecessarily broaden the API that's available to the
> clk providers, you can use EXPORT_SYMBOL_IF_KUNIT so that this is only
> available to the kunit tests.

Ah, good idea.

> Note that Chen-Yu posted a separate patch to add the includes for a
> separate test. The two patches will conflict since Stephen hasn't picked
> this up yet.
>
> https://lore.kernel.org/linux-clk/20260225083413.3384950-1-wenst@chromium.org/

Thanks for the warning, I will synchronize with Chen-Yu.

>>  static struct clk *__of_clk_get(struct device_node *np,
>>  				int index, const char *dev_id,
>> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
>> index a268d7b5d4cb..b814b45f1f7e 100644
>> --- a/drivers/clk/clk_test.c
>> +++ b/drivers/clk/clk_test.c
>> @@ -3541,10 +3541,134 @@ static struct kunit_suite clk_hw_get_dev_of_node_test_suite = {
>>  	.test_cases = clk_hw_get_dev_of_node_test_cases,
>>  };
>>  
>> +static const struct clk_init_data clk_parse_clkspec_1_init_data = {
>> +	.name = "clk_parse_clkspec_1",
>> +	.ops = &empty_clk_ops,
>> +};
>> +
>> +static const struct clk_init_data clk_parse_clkspec_2_init_data = {
>> +	.name = "clk_parse_clkspec_2",
>> +	.ops = &empty_clk_ops,
>> +};
>> +
>> +static struct clk_hw *kunit_clk_get(struct of_phandle_args *clkspec, void *data)
>> +{
>> +	return (struct clk_hw *)data;
>> +}
>> +
>> +struct clk_parse_clkspec_ctx {
>> +	struct device_node *prov1_np;
>> +	struct device_node *prov2_np;
>> +	struct device_node *cons_np;
>> +};
>> +
>> +static int clk_parse_clkspec_init(struct kunit *test)
>> +{
>> +	struct clk_parse_clkspec_ctx *ctx;
>> +	struct clk_hw *hw1, *hw2;
>> +
>> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>> +	test->priv = ctx;
>> +
>> +	KUNIT_ASSERT_EQ(test, 0, of_overlay_apply_kunit(test, kunit_clk_parse_clkspec));
>> +
>> +	/* Register provider 1 */
>> +	hw1 = kunit_kzalloc(test, sizeof(*hw1), GFP_KERNEL);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw1);
>> +	hw1->init = &clk_parse_clkspec_1_init_data;
>> +
>> +	ctx->prov1_np = of_find_compatible_node(NULL, NULL, "test,clock-provider1");
>> +	KUNIT_ASSERT_NOT_NULL(test, ctx->prov1_np);
>> +
>> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->prov1_np, hw1));
>> +	of_clk_add_hw_provider(ctx->prov1_np, kunit_clk_get, hw1);
>
> Can you just use of_clk_hw_simple_get() and drop kunit_clk_get()
> above?

I will try.

>> +	of_node_put(ctx->prov1_np);
>> +
>> +	/* Register provider 2 */
>> +	hw2 = kunit_kzalloc(test, sizeof(*hw2), GFP_KERNEL);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw2);
>> +	hw2->init = &clk_parse_clkspec_2_init_data;
>> +
>> +	ctx->prov2_np = of_find_compatible_node(NULL, NULL, "test,clock-provider2");
>> +	KUNIT_ASSERT_NOT_NULL(test, ctx->prov2_np);
>> +
>> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->prov2_np, hw2));
>> +	of_clk_add_hw_provider(ctx->prov2_np, kunit_clk_get, hw2);
>> +	of_node_put(ctx->prov2_np);
>> +
>> +	ctx->cons_np = of_find_compatible_node(NULL, NULL, "test,clock-consumer");
>> +	KUNIT_ASSERT_NOT_NULL(test, ctx->cons_np);
>> +
>> +	return 0;
>> +}
>> +
>> +static void clk_parse_clkspec_exit(struct kunit *test)
>> +{
>> +	struct clk_parse_clkspec_ctx *ctx = test->priv;
>> +
>> +	of_node_put(ctx->prov1_np);
>> +	of_node_put(ctx->prov2_np);
>
> Is there a double free of prov1_np and prov2_np? If this is dropped from
> the test exit, then they should't need to be in the ctx struct.

These two calls increment the refcount on the node:
- of_find_compatible_node()
- of_clk_add_hw_provider()

However this makes me realize maybe I should call of_clk_del_provider()
in the exit() function. In any case, I believe keeping a reference over
the nodes during the test is correct and if there is an of_node_put()
call to remove, it should be the on in the _init().

Thanks for pointing this out!
Miquèl

Re: [PATCH 06/16] clk: tests: Add clk_parse_clkspec() Kunit testing

[Brian Masney]

Hi Miquel,

On Wed, Apr 01, 2026 at 10:59:20AM +0200, Miquel Raynal wrote:
> >> +	of_node_put(ctx->prov1_np);
> >> +
> >> +	/* Register provider 2 */
> >> +	hw2 = kunit_kzalloc(test, sizeof(*hw2), GFP_KERNEL);
> >> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw2);
> >> +	hw2->init = &clk_parse_clkspec_2_init_data;
> >> +
> >> +	ctx->prov2_np = of_find_compatible_node(NULL, NULL, "test,clock-provider2");
> >> +	KUNIT_ASSERT_NOT_NULL(test, ctx->prov2_np);
> >> +
> >> +	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->prov2_np, hw2));
> >> +	of_clk_add_hw_provider(ctx->prov2_np, kunit_clk_get, hw2);
> >> +	of_node_put(ctx->prov2_np);
> >> +
> >> +	ctx->cons_np = of_find_compatible_node(NULL, NULL, "test,clock-consumer");
> >> +	KUNIT_ASSERT_NOT_NULL(test, ctx->cons_np);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void clk_parse_clkspec_exit(struct kunit *test)
> >> +{
> >> +	struct clk_parse_clkspec_ctx *ctx = test->priv;
> >> +
> >> +	of_node_put(ctx->prov1_np);
> >> +	of_node_put(ctx->prov2_np);
> >
> > Is there a double free of prov1_np and prov2_np? If this is dropped from
> > the test exit, then they should't need to be in the ctx struct.
> 
> These two calls increment the refcount on the node:
> - of_find_compatible_node()
> - of_clk_add_hw_provider()
> 
> However this makes me realize maybe I should call of_clk_del_provider()
> in the exit() function. In any case, I believe keeping a reference over
> the nodes during the test is correct and if there is an of_node_put()
> call to remove, it should be the on in the _init().

Take a look at drivers/clk/clk_kunit_helpers.c.
of_clk_add_hw_provider_kunit() will call of_clk_del_provider() for you
via of_clk_del_provider_wrapper.

Brian

Re: [PATCH 08/16] clk: Improve a couple of comments

[Brian Masney]

On Fri, Mar 27, 2026 at 09:09:30PM +0100, Miquel Raynal (Schneider Electric) wrote:
> Avoid mentioning the function names directly in the comments, it makes
> them easily out of sync with the rest of the code. Use a more generic
> wording.
> 
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

Reviewed-by: Brian Masney <bmasney@redhat.com>

Re: [PATCH 09/16] clk: Use the generic OF phandle parsing in only one place

[Brian Masney]

On Fri, Mar 27, 2026 at 09:09:31PM +0100, Miquel Raynal (Schneider Electric) wrote:
> There should be one single entry in the OF world, so that the way we
> parse the DT is always the same. make sure this is the case by avoid
> calling of_parse_phandle_with_args() from of_clk_get_parent_name(). This
> is even more relevant as we currently fail to parse clock-ranges. As a
> result, it seems to be safer to directly call of_parse_clkspec() there.
> 
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  drivers/clk/clk.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 591c0780b61e..93e33ff30f3a 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5375,8 +5375,7 @@ const char *of_clk_get_parent_name(const struct device_node *np, int index)
>  	int count;
>  	struct clk *clk;
>  
> -	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> -					&clkspec);
> +	rc = of_parse_clkspec(np, index, NULL, &clkspec);
>  	if (rc)
>  		return NULL;

Reviewed-by: Brian Masney <bmasney@redhat.com>

In case a Fixes tag is warranted, it's not exactly clear what should be
used. This was introduced in commit 766e6a4ec602 ("clk: add DT clock
binding support") in 2012. However of_parse_clkspec was introduced in
commit 4472287a3b2f5 ("clk: Introduce of_clk_get_hw_from_clkspec()") in
2018.

Re: [PATCH 09/16] clk: Use the generic OF phandle parsing in only one place

[Miquel Raynal]

On 30/03/2026 at 11:01:59 -04, Brian Masney <bmasney@redhat.com> wrote:

> On Fri, Mar 27, 2026 at 09:09:31PM +0100, Miquel Raynal (Schneider Electric) wrote:
>> There should be one single entry in the OF world, so that the way we
>> parse the DT is always the same. make sure this is the case by avoid
>> calling of_parse_phandle_with_args() from of_clk_get_parent_name(). This
>> is even more relevant as we currently fail to parse clock-ranges. As a
>> result, it seems to be safer to directly call of_parse_clkspec() there.
>> 
>> Suggested-by: Stephen Boyd <sboyd@kernel.org>
>> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
>> ---
>>  drivers/clk/clk.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 591c0780b61e..93e33ff30f3a 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -5375,8 +5375,7 @@ const char *of_clk_get_parent_name(const struct device_node *np, int index)
>>  	int count;
>>  	struct clk *clk;
>>  
>> -	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
>> -					&clkspec);
>> +	rc = of_parse_clkspec(np, index, NULL, &clkspec);
>>  	if (rc)
>>  		return NULL;
>
> Reviewed-by: Brian Masney <bmasney@redhat.com>
>
> In case a Fixes tag is warranted, it's not exactly clear what should be
> used. This was introduced in commit 766e6a4ec602 ("clk: add DT clock
> binding support") in 2012. However of_parse_clkspec was introduced in
> commit 4472287a3b2f5 ("clk: Introduce of_clk_get_hw_from_clkspec()") in
> 2018.

I didn't plan to add a Fixes here, but I can. In this case I would go
for:

    commit 4472287a3b2f5 ("clk: Introduce of_clk_get_hw_from_clkspec()")

Thanks,
Miquèl

Re: [PATCH 10/16] clk: Add support for clock nexus dt bindings

[Brian Masney]

On Fri, Mar 27, 2026 at 09:09:32PM +0100, Miquel Raynal (Schneider Electric) wrote:
> A nexus node is some kind of parent device abstracting the outer
> connections. They are particularly useful for describing connectors-like
> interfaces but not only. Certain IP blocks will typically include inner
> blocks and distribute resources to them.
> 
> In the case of clocks, there is already the concept of clock controller,
> but this usually indicates some kind of control over the said clock,
> ie. gate or rate control. When there is none of this, an existing
> approach is to reference the upper clock, which is wrong from a hardware
> point of view.
> 
> Nexus nodes are already part of the device-tree specification and clocks
> are already mentioned:
> https://github.com/devicetree-org/devicetree-specification/blob/v0.4/source/chapter2-devicetree-basics.rst#nexus-nodes-and-specifier-mapping
> 
> Following the introductions of nexus nodes support for interrupts, gpios
> and pwms, here is the same logic applied again to the clk subsystem,
> just by transitioning from of_parse_phandle_with_args() to
> of_parse_phandle_with_args_map():
> 
> * Nexus OF support:
> commit bd6f2fd5a1d5 ("of: Support parsing phandle argument lists through a nexus node")
> * GPIO adoption:
> commit c11e6f0f04db ("gpio: Support gpio nexus dt bindings")
> * PWM adoption:
> commit e71e46a6f19c ("pwm: Add support for pwm nexus dt bindings")
> 
> Expected Nexus properties supported:
> - clock-map: maps inner clocks to inlet clocks,
> - clock-map-mask: specifier cell(s) which will be remapped,
> - clock-map-pass-thru: specifier cell(s) not used for remapping,
>   forwarded as-is.
> 
> In my own usage I had to deal with controllers where clock-map-mask and
> clock-map-pass-thru were not relevant, but here is a made up example
> showing how all these properties could go together:
> 
> Example:
>     soc_clk: clock-controller {
>         #clock-cells = <2>;
>     };
> 
>     container: container {
>         #clock-cells = <2>;
>         clock-map = <0 0 &soc_clk 2 0>,
>                     <1 0 &soc_clk 6 0>;
>         clock-map-mask = <0xffffffff 0x0>;
>         clock-map-pass-thru = <0x0 0xffffffff>;
> 
>         child-device {
>             clocks = <&container 1 0>;
> 	    /* This is equivalent to <&soc_clk 6 0> */
>         };
>     };
> 
> The child device does not need to know about the outer implementation,
> and only knows about what the nexus provides. The nexus acts as a
> pass-through, with no extra control.
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> Reviewed-by: Herve Codina <herve.codina@bootlin.com>

Reviewed-by: Brian Masney <bmasney@redhat.com>

Re: [PATCH 10/16] clk: Add support for clock nexus dt bindings

[Brian Masney]

On Fri, Mar 27, 2026 at 09:09:32PM +0100, Miquel Raynal (Schneider Electric) wrote:
> A nexus node is some kind of parent device abstracting the outer
> connections. They are particularly useful for describing connectors-like
> interfaces but not only. Certain IP blocks will typically include inner
> blocks and distribute resources to them.
> 
> In the case of clocks, there is already the concept of clock controller,
> but this usually indicates some kind of control over the said clock,
> ie. gate or rate control. When there is none of this, an existing
> approach is to reference the upper clock, which is wrong from a hardware
> point of view.
> 
> Nexus nodes are already part of the device-tree specification and clocks
> are already mentioned:
> https://github.com/devicetree-org/devicetree-specification/blob/v0.4/source/chapter2-devicetree-basics.rst#nexus-nodes-and-specifier-mapping
> 
> Following the introductions of nexus nodes support for interrupts, gpios
> and pwms, here is the same logic applied again to the clk subsystem,
> just by transitioning from of_parse_phandle_with_args() to
> of_parse_phandle_with_args_map():
> 
> * Nexus OF support:
> commit bd6f2fd5a1d5 ("of: Support parsing phandle argument lists through a nexus node")
> * GPIO adoption:
> commit c11e6f0f04db ("gpio: Support gpio nexus dt bindings")
> * PWM adoption:
> commit e71e46a6f19c ("pwm: Add support for pwm nexus dt bindings")
> 
> Expected Nexus properties supported:
> - clock-map: maps inner clocks to inlet clocks,
> - clock-map-mask: specifier cell(s) which will be remapped,
> - clock-map-pass-thru: specifier cell(s) not used for remapping,
>   forwarded as-is.
> 
> In my own usage I had to deal with controllers where clock-map-mask and
> clock-map-pass-thru were not relevant, but here is a made up example
> showing how all these properties could go together:
> 
> Example:
>     soc_clk: clock-controller {
>         #clock-cells = <2>;
>     };
> 
>     container: container {
>         #clock-cells = <2>;
>         clock-map = <0 0 &soc_clk 2 0>,
>                     <1 0 &soc_clk 6 0>;
>         clock-map-mask = <0xffffffff 0x0>;
>         clock-map-pass-thru = <0x0 0xffffffff>;
> 
>         child-device {
>             clocks = <&container 1 0>;
> 	    /* This is equivalent to <&soc_clk 6 0> */
>         };
>     };
> 
> The child device does not need to know about the outer implementation,
> and only knows about what the nexus provides. The nexus acts as a
> pass-through, with no extra control.
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> Reviewed-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/clk/clk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 93e33ff30f3a..196ba727e84b 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5218,8 +5218,8 @@ static int of_parse_clkspec(const struct device_node *np, int index,
>  		 */
>  		if (name)
>  			index = of_property_match_string(np, "clock-names", name);
> -		ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
> -						 index, out_args);
> +		ret = of_parse_phandle_with_args_map(np, "clocks", "clock",
> +						     index, out_args);

Before I left my Reviewed-by, I should have double checked Sashiko. It
has several questions about this patch. The first is:

    Are there other places in the clock framework that need to transition to the
    new map API to ensure assigned clocks work?
    
    For instance, assigned-clocks and assigned-clock-parents are parsed in
    drivers/clk/clk-conf.c using of_parse_phandle_with_args(). If a device
    specifies an assigned clock that routes through a nexus node, will it fail
    to configure because the map is not traversed?

https://sashiko.dev/#/patchset/20260327-schneider-v7-0-rc1-crypto-v1-0-5e6ff7853994%40bootlin.com?patch=12563

Brian

Re: [PATCH 10/16] clk: Add support for clock nexus dt bindings

[Miquel Raynal]

Hello Brian,

First, thanks for the whole review.

On 30/03/2026 at 11:16:44 -04, Brian Masney <bmasney@redhat.com> wrote:
>> -		ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
>> -						 index, out_args);
>> +		ret = of_parse_phandle_with_args_map(np, "clocks", "clock",
>> +						     index, out_args);
>
> Before I left my Reviewed-by, I should have double checked Sashiko. It
> has several questions about this patch. The first is:
>
>     Are there other places in the clock framework that need to transition to the
>     new map API to ensure assigned clocks work?
>     
>     For instance, assigned-clocks and assigned-clock-parents are parsed in
>     drivers/clk/clk-conf.c using of_parse_phandle_with_args(). If a device
>     specifies an assigned clock that routes through a nexus node, will it fail
>     to configure because the map is not traversed?

The goal of the nexus node is to isolate what is behind. Are
assigned-clocks et al. supposed to traverse a nexus node? I am tempted
to say "no", but I'm open to discussing this ofc.

> https://sashiko.dev/#/patchset/20260327-schneider-v7-0-rc1-crypto-v1-0-5e6ff7853994%40bootlin.com?patch=12563

I have mixed feelings concerning Sashiko's feedback. I will go through
that page nevertheless, there are interesting comments in there.

Thanks,
Miquèl

Re: [PATCH 10/16] clk: Add support for clock nexus dt bindings

[Brian Masney]

Hi Miquel,

On Wed, Apr 01, 2026 at 10:47:51AM +0200, Miquel Raynal wrote:
> First, thanks for the whole review.
> 
> On 30/03/2026 at 11:16:44 -04, Brian Masney <bmasney@redhat.com> wrote:
> >> -		ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
> >> -						 index, out_args);
> >> +		ret = of_parse_phandle_with_args_map(np, "clocks", "clock",
> >> +						     index, out_args);
> >
> > Before I left my Reviewed-by, I should have double checked Sashiko. It
> > has several questions about this patch. The first is:
> >
> >     Are there other places in the clock framework that need to transition to the
> >     new map API to ensure assigned clocks work?
> >     
> >     For instance, assigned-clocks and assigned-clock-parents are parsed in
> >     drivers/clk/clk-conf.c using of_parse_phandle_with_args(). If a device
> >     specifies an assigned clock that routes through a nexus node, will it fail
> >     to configure because the map is not traversed?
> 
> The goal of the nexus node is to isolate what is behind. Are
> assigned-clocks et al. supposed to traverse a nexus node? I am tempted
> to say "no", but I'm open to discussing this ofc.

I agree that it's not needed as well, however I want to defer to
Stephen's expertise here. I mainly brought this up trying to help him
with reviews.

> > https://sashiko.dev/#/patchset/20260327-schneider-v7-0-rc1-crypto-v1-0-5e6ff7853994%40bootlin.com?patch=12563
> 
> I have mixed feelings concerning Sashiko's feedback. I will go through
> that page nevertheless, there are interesting comments in there.

I have mixed feelings as well about the feedback from Sashiko. It finds
issues, however not all of the feedback has been helpful. On the whole,
I'm glad that it's available.

Brian