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

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

Re: [PATCH 01/16] dt-bindings: clock: Introduce nexus nodes

[Rob Herring]

On Fri, Mar 27, 2026 at 09:09:23PM +0100, Miquel Raynal (Schneider Electric) wrote:
> Hardware containers can just decouple external resources like clock
> without any more control. Nexus nodes already exist for PWM and GPIOs,
> add a binding to allow them for clocks as well.
> 
> No examples are given, the file is litteraly a copy-paste from Hervé
> Codina's work on PWM Nexus nodes, hence we just point to the examples
> there which already illustrate very clearly the concept of the various
> properties.
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  .../bindings/clock/clock-nexus-node.yaml           | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-nexus-node.yaml b/Documentation/devicetree/bindings/clock/clock-nexus-node.yaml
> new file mode 100644
> index 000000000000..f07e2972e8aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clock-nexus-node.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/clock-nexus-node.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Clock Nexus
> +
> +description: >
> +  A nexus node allows to remap a phandle list in a consumer node through a
> +  container or a connector node in a generic way. With this remapping,
> +  the consumer node needs to know only about the nexus node. Resources
> +  behind the nexus node are decoupled by the nexus node itself.
> +
> +maintainers:
> +  - Miquel Raynal <miquel.raynal@bootlin.com>
> +
> +select: true
> +
> +properties:
> +  '#clock-cells': true

No need for this.

> +
> +  clock-map:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +
> +  clock-map-mask:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +
> +  clock-map-pass-thru:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array

I think this can be omitted because there aren't common cell definitions 
for clocks like there are for GPIO and PWM.

> +
> +dependentRequired:
> +  clock-map: ['#clock-cells']
> +  clock-map-mask: [ clock-map ]
> +  clock-map-pass-thru: [ clock-map ]
> +
> +additionalProperties: true
> +
> +# See the original pwm-nexus-node.yaml description for examples
> 
> -- 
> 2.51.1
> 

Re: [PATCH 16/16] ARM: dts: renesas: r9a06g032: Describe the EIP-150 block

[Rob Herring]

On Fri, Mar 27, 2026 at 09:09:38PM +0100, Miquel Raynal (Schneider Electric) wrote:
> The EIP-150 is composed of 3 blocks:
> * An interrupt controller named EIP-201 AIC
>     - fed by a clock coming from the EIP-150
>     - connected to the main GIC
> * A random number generator named EIP-76
>     - fed by a clock coming from the EIP-150
>     - signalling interrupts through the AIC
> * A public key accelerator engine named EIP-28
>     - Fed by a clock coming from the EIP-150
>     - Signalling interrupts through the AIC
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  arch/arm/boot/dts/renesas/r9a06g032.dtsi | 42 ++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/renesas/r9a06g032.dtsi b/arch/arm/boot/dts/renesas/r9a06g032.dtsi
> index f4f760aff28b..6aaa93ed03d6 100644
> --- a/arch/arm/boot/dts/renesas/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/renesas/r9a06g032.dtsi
> @@ -8,6 +8,7 @@
>  
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/r9a06g032-sysctrl.h>
> +#include <dt-bindings/interrupt-controller/inside-secure,safexcel-eip201.h>
>  
>  / {
>  	compatible = "renesas,r9a06g032";
> @@ -170,6 +171,47 @@ usb@2,0 {
>  			};
>  		};
>  
> +		eip150: bus@40040000 {
> +			compatible = "inside-secure,safexcel-eip150", "simple-pm-bus";
> +			clocks = <&sysctrl R9A06G032_HCLK_CRYPTO_EIP150>;
> +			#clock-cells = <0>;
> +			clock-map = <&sysctrl R9A06G032_HCLK_CRYPTO_EIP150>;

I don't get why you need clock-map here. Why can't you just put this 
clock in each child node?

Rob

Re: [PATCH 05/16] dt-bindings: bus: eip150: Describe the EIP-150 container node

[Rob Herring]

On Fri, Mar 27, 2026 at 09:09:27PM +0100, Miquel Raynal (Schneider Electric) wrote:
> Part of Inside-Secure's SafeXcel family, the EIP-150 is some kind of
> container node composed of:
> - a public key accelerator,
> - random number generator,
> - an interrupt controller.
> 
> It also acts as proxy for the clocks.
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  .../bus/inside-secure,safexcel-eip150.yaml         | 58 ++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/bus/inside-secure,safexcel-eip150.yaml b/Documentation/devicetree/bindings/bus/inside-secure,safexcel-eip150.yaml
> new file mode 100644
> index 000000000000..1b3d83a852f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/inside-secure,safexcel-eip150.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bus/inside-secure,safexcel-eip150.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Inside-Secure SafeXcel EIP-150 container
> +
> +maintainers:
> +  - Miquel Raynal <miquel.raynal@bootlin.com>
> +
> +description:
> +  The EIP-150 is a hardware container, it has its own interrupt
> +  controller inside to which a random number generator and a public key
> +  accelerator are wired.
> +
> +allOf:
> +  - $ref: simple-pm-bus.yaml#
> +  - $ref: /schemas/clock/clock-nexus-node.yaml#

Generally, if a schema has 'select: true', you don't reference it as it 
has already been applied. And you have to list the properties here 
anyways because you need to define how many #clock-cells for example.

> +
> +properties:
> +  compatible:
> +    items:
> +      - const: inside-secure,safexcel-eip150
> +      - {} # simple-pm-bus, but not listed here to avoid false select
> +
> +  clocks:
> +    minItems: 1

Seems to me you are adding this to satisfy simple-pm-bus. Maybe this 
should just be simple-bus?

> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges: true
> +
> +patternProperties:
> +  "^interrupt-controller@[0-9a-f]+$":
> +    type: object
> +    $ref: /schemas/interrupt-controller/inside-secure,safexcel-eip201.yaml#

Better to just list a compatible you require. Like this, the schema is 
applied twice.

> +
> +  "^rng@[0-9a-f]+$":
> +    type: object
> +    $ref: /schemas/rng/inside-secure,safexcel-eip76.yaml#
> +
> +  "^crypto@[0-9a-f]+$":
> +    type: object
> +    $ref: /schemas/crypto/inside-secure,safexcel-eip28.yaml#
> +
> +required:
> +  - compatible
> +  - clocks

> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges

The bus schema requires all these already.

Rob

Re: [PATCH 02/16] dt-bindings: interrupt-controller: Describe EIP-201 AIC

[Rob Herring (Arm)]

On Fri, 27 Mar 2026 21:09:24 +0100, Miquel Raynal (Schneider Electric) wrote:
> The EIP-201 Advanced Interrupt Controller is part of a bigger container
> block from Inside Secure nicely named EIP-150. It typically takes one
> clock from the EIP-150 and offers basic controls through a few simple
> registers.
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  .../inside-secure,safexcel-eip201.yaml             | 41 ++++++++++++++++++++++
>  .../inside-secure,safexcel-eip201.h                | 14 ++++++++
>  2 files changed, 55 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 03/16] dt-bindings: rng: Rename the title of the EIP-76 file

[Rob Herring (Arm)]

On Fri, 27 Mar 2026 21:09:25 +0100, Miquel Raynal (Schneider Electric) wrote:
> Be a little more precise in the title by giving the family name and the
> own name of the hardware block. Despite the original compatibles, this
> file describes a SafeXcel EIP-76 hardware random number generator.
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  Documentation/devicetree/bindings/rng/inside-secure,safexcel-eip76.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 04/16] dt-bindings: crypto: eip28: Describe EIP-28 PKA

[Rob Herring]

On Fri, Mar 27, 2026 at 09:09:26PM +0100, Miquel Raynal (Schneider Electric) wrote:
> Describe the Public Key Accelerator named EIP-28 from Inside-Secure,
> part of the SafeXcel family, it is typically included in a bigger
> hardware container named EIP-150.
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  .../crypto/inside-secure,safexcel-eip28.yaml       | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/crypto/inside-secure,safexcel-eip28.yaml b/Documentation/devicetree/bindings/crypto/inside-secure,safexcel-eip28.yaml
> new file mode 100644
> index 000000000000..96acb257450a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/inside-secure,safexcel-eip28.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/crypto/inside-secure,safexcel-eip28.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Inside-Secure SafeXcel EIP-28 Public Key Accelerator
> +
> +maintainers:
> +  - Miquel Raynal <miquel.raynal@bootlin.com>
> +
> +properties:
> +  compatible:
> +    const: inside-secure,safexcel-eip28
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 3
> +
> +  clocks:
> +    minItems: 1

maxItems: 1

(only)

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +additionalProperties: false
> 
> -- 
> 2.51.1
> 

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 12/16] irqchip/eip201-aic: Add support for Safexcel EIP-201 AIC

[Geert Uytterhoeven]

Hi Miquel,

On Fri, 27 Mar 2026 at 21:11, Miquel Raynal (Schneider Electric)
<miquel.raynal@bootlin.com> wrote:
> Describe the EIP-201 Advanced Interrupt Controller from Inside Secure,
> typically found in a bigger block named EIP-150. This controller is
> rather simple and is driven using the generic irqchip model. Its
> own interrupt domain is limited to just a few interrupts connected to
> other inner blocks, such as a Random Number Generator and a Public Key
> Accelerator.
>
> The one I used receives only rising edge interrupts and uses its own
> logic to track them. It is theoretically possible to wire devices with
> level interrupts, but not in the context of the EIP-150.
>
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -826,4 +826,12 @@ config SUNPLUS_SP7021_INTC
>           chained controller, routing all interrupt source in P-Chip to
>           the primary controller on C-Chip.
>
> +config SAFEXCEL_EIP201_AIC
> +        tristate "Safexcel EIP201 AIC"

Is there any platform dependency that could be added here?

> +       select IRQ_DOMAIN
> +       help
> +         Support for the Advanced Interrupt Controller (AIC) typically
> +         inside Safexcel EIP150 IPs, gathering Public Key Accelerator
> +         and True Random Number Generator interrupts.
> +
>  endmenu

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