diff mbox series

[v8,8/8] clk: Add KUnit tests for clks registered with struct clk_parent_data

Message ID 20240718210513.3801024-9-sboyd@kernel.org (mailing list archive)
State Accepted
Commit 274aff8711b2e77c27bbda0ddc24caa39f154bfa
Headers show
Series clk: Add kunit tests for fixed rate and parent data | expand

Commit Message

Stephen Boyd July 18, 2024, 9:05 p.m. UTC
Test that clks registered with 'struct clk_parent_data' work as
intended and can find their parents.

Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Brendan Higgins <brendan.higgins@linux.dev>
Reviewed-by: David Gow <davidgow@google.com>
Cc: Rae Moar <rmoar@google.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/Kconfig                         |   2 +
 drivers/clk/Makefile                        |   4 +-
 drivers/clk/clk_parent_data_test.h          |  10 +
 drivers/clk/clk_test.c                      | 453 +++++++++++++++++++-
 drivers/clk/kunit_clk_parent_data_test.dtso |  28 ++
 5 files changed, 495 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clk/clk_parent_data_test.h
 create mode 100644 drivers/clk/kunit_clk_parent_data_test.dtso

Comments

Stephen Boyd July 29, 2024, 10:38 p.m. UTC | #1
Quoting Stephen Boyd (2024-07-18 14:05:07)
> Test that clks registered with 'struct clk_parent_data' work as
> intended and can find their parents.
> 
> Cc: Christian Marangi <ansuelsmth@gmail.com>
> Cc: Brendan Higgins <brendan.higgins@linux.dev>
> Reviewed-by: David Gow <davidgow@google.com>
> Cc: Rae Moar <rmoar@google.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---

Applied to clk-next
Guenter Roeck Sept. 27, 2024, 4:14 a.m. UTC | #2
Hi Stephen,

On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
> Test that clks registered with 'struct clk_parent_data' work as
> intended and can find their parents.
> 

When testing this on arm64, I see the error below. The error is only
seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
qemu parameter.

Any idea what might cause the problem ?

Thanks,
Guenter

---
[   20.464809]     KTAP version 1
[   20.464865]     # Subtest: clk_register_clk_parent_data_of
[   20.464936]     # module: clk_test
[   20.464979]     1..1
[   20.465098]         KTAP version 1
[   20.465208]         # Subtest: clk_register_clk_parent_data_of_test
[   20.468964] OF: overlay: find target, node: /fragment@0, path '/' not found
[   20.469558] OF: overlay: init_overlay_changeset() failed, ret = -22
[   20.470177]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
[   20.470177]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
[   20.470177]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
[   20.471793]         not ok 1 clk_parent_data_of_index_test
[   20.474095] OF: overlay: find target, node: /fragment@0, path '/' not found
[   20.474373] OF: overlay: init_overlay_changeset() failed, ret = -22
[   20.474737]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
[   20.474737]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
[   20.474737]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
[   20.477677]         not ok 2 clk_parent_data_of_fwname_test
[   20.479773] OF: overlay: find target, node: /fragment@0, path '/' not found
[   20.479941] OF: overlay: init_overlay_changeset() failed, ret = -22
[   20.480160]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
[   20.480160]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
[   20.480160]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
[   20.481513]         not ok 3 clk_parent_data_of_name_test
[   20.483711] OF: overlay: find target, node: /fragment@0, path '/' not found
[   20.483878] OF: overlay: init_overlay_changeset() failed, ret = -22
[   20.484100]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
[   20.484100]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
[   20.484100]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
[   20.485444]         not ok 4 clk_parent_data_of_fwname_name_test
[   20.487432] OF: overlay: find target, node: /fragment@0, path '/' not found
[   20.487600] OF: overlay: init_overlay_changeset() failed, ret = -22
[   20.487841]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
[   20.487841]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
[   20.487841]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
[   20.489207]         not ok 5 clk_parent_data_of_index_name_priority_test
[   20.490998] OF: overlay: find target, node: /fragment@0, path '/' not found
[   20.491504] OF: overlay: init_overlay_changeset() failed, ret = -22
[   20.491725]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
[   20.491725]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
[   20.491725]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
[   20.493053]         not ok 6 clk_parent_data_of_index_fwname_name_priority_test
[   20.493583]     # clk_register_clk_parent_data_of_test: pass:0 fail:6 skip:0 total:6
[   20.493701]     not ok 1 clk_register_clk_parent_data_of_test
[   20.493822] # clk_register_clk_parent_data_of: pass:0 fail:1 skip:0 total:1
[   20.493920] # Totals: pass:0 fail:6 skip:0 total:6
[   20.494032] not ok 49 clk_register_clk_parent_data_of
Guenter Roeck Sept. 27, 2024, 4:39 a.m. UTC | #3
On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
> Hi Stephen,
> 
> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
> > Test that clks registered with 'struct clk_parent_data' work as
> > intended and can find their parents.
> > 
> 
> When testing this on arm64, I see the error below. The error is only
> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
> qemu parameter.
> 
> Any idea what might cause the problem ?
> 
I noticed that the new overlay tests fail as well, also with "path '/' not
found".

[Maybe] answering my own question: I think the problem may be that there
is no devicetree file and thus no devicetree root when booting through
efi (in other words, of_root is NULL). Would it make sense to skip the
tests in that case ?

Thanks,
Guenter

> Thanks,
> Guenter
> 
> ---
> [   20.464809]     KTAP version 1
> [   20.464865]     # Subtest: clk_register_clk_parent_data_of
> [   20.464936]     # module: clk_test
> [   20.464979]     1..1
> [   20.465098]         KTAP version 1
> [   20.465208]         # Subtest: clk_register_clk_parent_data_of_test
> [   20.468964] OF: overlay: find target, node: /fragment@0, path '/' not found
> [   20.469558] OF: overlay: init_overlay_changeset() failed, ret = -22
> [   20.470177]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
> [   20.470177]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
> [   20.470177]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
> [   20.471793]         not ok 1 clk_parent_data_of_index_test
> [   20.474095] OF: overlay: find target, node: /fragment@0, path '/' not found
> [   20.474373] OF: overlay: init_overlay_changeset() failed, ret = -22
> [   20.474737]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
> [   20.474737]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
> [   20.474737]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
> [   20.477677]         not ok 2 clk_parent_data_of_fwname_test
> [   20.479773] OF: overlay: find target, node: /fragment@0, path '/' not found
> [   20.479941] OF: overlay: init_overlay_changeset() failed, ret = -22
> [   20.480160]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
> [   20.480160]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
> [   20.480160]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
> [   20.481513]         not ok 3 clk_parent_data_of_name_test
> [   20.483711] OF: overlay: find target, node: /fragment@0, path '/' not found
> [   20.483878] OF: overlay: init_overlay_changeset() failed, ret = -22
> [   20.484100]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
> [   20.484100]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
> [   20.484100]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
> [   20.485444]         not ok 4 clk_parent_data_of_fwname_name_test
> [   20.487432] OF: overlay: find target, node: /fragment@0, path '/' not found
> [   20.487600] OF: overlay: init_overlay_changeset() failed, ret = -22
> [   20.487841]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
> [   20.487841]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
> [   20.487841]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
> [   20.489207]         not ok 5 clk_parent_data_of_index_name_priority_test
> [   20.490998] OF: overlay: find target, node: /fragment@0, path '/' not found
> [   20.491504] OF: overlay: init_overlay_changeset() failed, ret = -22
> [   20.491725]     # clk_register_clk_parent_data_of_test: ASSERTION FAILED at drivers/clk/clk_test.c:2760
> [   20.491725]     Expected 0 == ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }), but
> [   20.491725]         ({ extern uint8_t __dtbo_kunit_clk_parent_data_test_begin[]; extern uint8_t __dtbo_kunit_clk_parent_data_test_end[]; __of_overlay_apply_kunit((test), __dtbo_kunit_clk_parent_data_test_begin, __dtbo_kunit_clk_parent_data_test_end); }) == -22 (0xffffffffffffffea)
> [   20.493053]         not ok 6 clk_parent_data_of_index_fwname_name_priority_test
> [   20.493583]     # clk_register_clk_parent_data_of_test: pass:0 fail:6 skip:0 total:6
> [   20.493701]     not ok 1 clk_register_clk_parent_data_of_test
> [   20.493822] # clk_register_clk_parent_data_of: pass:0 fail:1 skip:0 total:1
> [   20.493920] # Totals: pass:0 fail:6 skip:0 total:6
> [   20.494032] not ok 49 clk_register_clk_parent_data_of
Guenter Roeck Sept. 27, 2024, 4:19 p.m. UTC | #4
Copying devicetree maintainers.

On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
> > Hi Stephen,
> > 
> > On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
> > > Test that clks registered with 'struct clk_parent_data' work as
> > > intended and can find their parents.
> > > 
> > 
> > When testing this on arm64, I see the error below. The error is only
> > seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
> > qemu parameter.
> > 
> > Any idea what might cause the problem ?
> > 
> I noticed that the new overlay tests fail as well, also with "path '/' not
> found".
> 
> [Maybe] answering my own question: I think the problem may be that there
> is no devicetree file and thus no devicetree root when booting through
> efi (in other words, of_root is NULL). Would it make sense to skip the
> tests in that case ?
> 

The problem is that of_root is not initialized in arm64 boots if ACPI
is enabled.

From arch/arm64/kernel/setup.c:setup_arch():

	if (acpi_disabled)
		unflatten_device_tree();		// initializes of_root

ACPI is enabled if the system boots from EFI. This also affects
CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
fails the test if it doesn't. 

I think those tests need to add a check for this condition, or affected
machines won't be able to run those unit tests. The obvious solution would
be to check if of_root is set, but then the associated test case in
CONFIG_OF_KUNIT_TEST would not make sense.

Any suggestions ?

Thanks,
Guenter
Shuah Khan Sept. 27, 2024, 8:45 p.m. UTC | #5
On 9/27/24 10:19, Guenter Roeck wrote:
> Copying devicetree maintainers.
> 
> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
>>> Hi Stephen,
>>>
>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
>>>> Test that clks registered with 'struct clk_parent_data' work as
>>>> intended and can find their parents.
>>>>
>>>
>>> When testing this on arm64, I see the error below. The error is only
>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
>>> qemu parameter.
>>>
>>> Any idea what might cause the problem ?
>>>
>> I noticed that the new overlay tests fail as well, also with "path '/' not
>> found".
>>
>> [Maybe] answering my own question: I think the problem may be that there
>> is no devicetree file and thus no devicetree root when booting through
>> efi (in other words, of_root is NULL). Would it make sense to skip the
>> tests in that case ?
>>
> 
> The problem is that of_root is not initialized in arm64 boots if ACPI
> is enabled.
> 
>  From arch/arm64/kernel/setup.c:setup_arch():
> 
> 	if (acpi_disabled)
> 		unflatten_device_tree();		// initializes of_root
> 
> ACPI is enabled if the system boots from EFI. This also affects
> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
> fails the test if it doesn't.
> 
> I think those tests need to add a check for this condition, or affected
> machines won't be able to run those unit tests. The obvious solution would
> be to check if of_root is set, but then the associated test case in
> CONFIG_OF_KUNIT_TEST would not make sense.
> 
> Any suggestions ?
> 

Would it work if these tests check if acpi_disabled and skip if it isn't
disabled? It might be low overhead condition to check from these tests.

acpi_disabled is exported:

arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled);

thanks,
-- Shuah
Guenter Roeck Sept. 28, 2024, 12:08 a.m. UTC | #6
On 9/27/24 13:45, Shuah Khan wrote:
> On 9/27/24 10:19, Guenter Roeck wrote:
>> Copying devicetree maintainers.
>>
>> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
>>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
>>>> Hi Stephen,
>>>>
>>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
>>>>> Test that clks registered with 'struct clk_parent_data' work as
>>>>> intended and can find their parents.
>>>>>
>>>>
>>>> When testing this on arm64, I see the error below. The error is only
>>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
>>>> qemu parameter.
>>>>
>>>> Any idea what might cause the problem ?
>>>>
>>> I noticed that the new overlay tests fail as well, also with "path '/' not
>>> found".
>>>
>>> [Maybe] answering my own question: I think the problem may be that there
>>> is no devicetree file and thus no devicetree root when booting through
>>> efi (in other words, of_root is NULL). Would it make sense to skip the
>>> tests in that case ?
>>>
>>
>> The problem is that of_root is not initialized in arm64 boots if ACPI
>> is enabled.
>>
>>  From arch/arm64/kernel/setup.c:setup_arch():
>>
>>     if (acpi_disabled)
>>         unflatten_device_tree();        // initializes of_root
>>
>> ACPI is enabled if the system boots from EFI. This also affects
>> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
>> fails the test if it doesn't.
>>
>> I think those tests need to add a check for this condition, or affected
>> machines won't be able to run those unit tests. The obvious solution would
>> be to check if of_root is set, but then the associated test case in
>> CONFIG_OF_KUNIT_TEST would not make sense.
>>
>> Any suggestions ?
>>
> 
> Would it work if these tests check if acpi_disabled and skip if it isn't
> disabled? It might be low overhead condition to check from these tests.
> 
> acpi_disabled is exported:
> 
> arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled);
> 

I don't think that would work. Looking through the use of acpi_init,
I don't think that of_root is always NULL when acpi_init is false; that
just happens to be the case on arm64 when booting through efi.
However, even arm64 has the following code.

         if (acpi_disabled)
                 psci_dt_init();
         else
                 psci_acpi_init();

While psci_dt_init() doesn't set of_root, it does try to do a devicetree
match. So there must be some other condition where acpi_disabled is set
but of_root is set anyway. I just have not found that code path.

Guenter
Guenter Roeck Sept. 28, 2024, 5:31 p.m. UTC | #7
On 9/27/24 17:08, Guenter Roeck wrote:
> On 9/27/24 13:45, Shuah Khan wrote:
>> On 9/27/24 10:19, Guenter Roeck wrote:
>>> Copying devicetree maintainers.
>>>
>>> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
>>>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
>>>>> Hi Stephen,
>>>>>
>>>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
>>>>>> Test that clks registered with 'struct clk_parent_data' work as
>>>>>> intended and can find their parents.
>>>>>>
>>>>>
>>>>> When testing this on arm64, I see the error below. The error is only
>>>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
>>>>> qemu parameter.
>>>>>
>>>>> Any idea what might cause the problem ?
>>>>>
>>>> I noticed that the new overlay tests fail as well, also with "path '/' not
>>>> found".
>>>>
>>>> [Maybe] answering my own question: I think the problem may be that there
>>>> is no devicetree file and thus no devicetree root when booting through
>>>> efi (in other words, of_root is NULL). Would it make sense to skip the
>>>> tests in that case ?
>>>>
>>>
>>> The problem is that of_root is not initialized in arm64 boots if ACPI
>>> is enabled.
>>>
>>>  From arch/arm64/kernel/setup.c:setup_arch():
>>>
>>>     if (acpi_disabled)
>>>         unflatten_device_tree();        // initializes of_root
>>>
>>> ACPI is enabled if the system boots from EFI. This also affects
>>> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
>>> fails the test if it doesn't.
>>>
>>> I think those tests need to add a check for this condition, or affected
>>> machines won't be able to run those unit tests. The obvious solution would
>>> be to check if of_root is set, but then the associated test case in
>>> CONFIG_OF_KUNIT_TEST would not make sense.
>>>
>>> Any suggestions ?
>>>
>>
>> Would it work if these tests check if acpi_disabled and skip if it isn't
>> disabled? It might be low overhead condition to check from these tests.
>>
>> acpi_disabled is exported:
>>
>> arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>> arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>> arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>> arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled);
>>
> 
> I don't think that would work. Looking through the use of acpi_init,
> I don't think that of_root is always NULL when acpi_init is false; that
> just happens to be the case on arm64 when booting through efi.
> However, even arm64 has the following code.
> 
>          if (acpi_disabled)
>                  psci_dt_init();
>          else
>                  psci_acpi_init();
> 
> While psci_dt_init() doesn't set of_root, it does try to do a devicetree
> match. So there must be some other condition where acpi_disabled is set
> but of_root is set anyway. I just have not found that code path.
> 

I ended up disabling all affected unit tests for arm64. I'll do the same
for other architectures if I encounter the problem there as well.

Unfortunately that includes all clock unit tests because the tests requiring
devicetree support can not be enabled/disabled separately, but that can't be
helped and is still better than "mandatory" failures.

Guenter
Shuah Khan Sept. 28, 2024, 5:54 p.m. UTC | #8
On 9/28/24 11:31, Guenter Roeck wrote:
> On 9/27/24 17:08, Guenter Roeck wrote:
>> On 9/27/24 13:45, Shuah Khan wrote:
>>> On 9/27/24 10:19, Guenter Roeck wrote:
>>>> Copying devicetree maintainers.
>>>>
>>>> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
>>>>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
>>>>>> Hi Stephen,
>>>>>>
>>>>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
>>>>>>> Test that clks registered with 'struct clk_parent_data' work as
>>>>>>> intended and can find their parents.
>>>>>>>
>>>>>>
>>>>>> When testing this on arm64, I see the error below. The error is only
>>>>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
>>>>>> qemu parameter.
>>>>>>
>>>>>> Any idea what might cause the problem ?
>>>>>>
>>>>> I noticed that the new overlay tests fail as well, also with "path '/' not
>>>>> found".
>>>>>
>>>>> [Maybe] answering my own question: I think the problem may be that there
>>>>> is no devicetree file and thus no devicetree root when booting through
>>>>> efi (in other words, of_root is NULL). Would it make sense to skip the
>>>>> tests in that case ?
>>>>>
>>>>
>>>> The problem is that of_root is not initialized in arm64 boots if ACPI
>>>> is enabled.
>>>>
>>>>  From arch/arm64/kernel/setup.c:setup_arch():
>>>>
>>>>     if (acpi_disabled)
>>>>         unflatten_device_tree();        // initializes of_root
>>>>
>>>> ACPI is enabled if the system boots from EFI. This also affects
>>>> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
>>>> fails the test if it doesn't.
>>>>
>>>> I think those tests need to add a check for this condition, or affected
>>>> machines won't be able to run those unit tests. The obvious solution would
>>>> be to check if of_root is set, but then the associated test case in
>>>> CONFIG_OF_KUNIT_TEST would not make sense.
>>>>
>>>> Any suggestions ?
>>>>
>>>
>>> Would it work if these tests check if acpi_disabled and skip if it isn't
>>> disabled? It might be low overhead condition to check from these tests.
>>>
>>> acpi_disabled is exported:
>>>
>>> arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>> arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>> arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>> arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled);
>>>
>>
>> I don't think that would work. Looking through the use of acpi_init,
>> I don't think that of_root is always NULL when acpi_init is false; that
>> just happens to be the case on arm64 when booting through efi.
>> However, even arm64 has the following code.
>>
>>          if (acpi_disabled)
>>                  psci_dt_init();
>>          else
>>                  psci_acpi_init();
>>
>> While psci_dt_init() doesn't set of_root, it does try to do a devicetree
>> match. So there must be some other condition where acpi_disabled is set
>> but of_root is set anyway. I just have not found that code path.
>>
> 
> I ended up disabling all affected unit tests for arm64. I'll do the same
> for other architectures if I encounter the problem there as well.
> 
> Unfortunately that includes all clock unit tests because the tests requiring
> devicetree support can not be enabled/disabled separately, but that can't be
> helped and is still better than "mandatory" failures.
> 

I am hoping Stephen will have a solution for this problem. In the meantime,
I will look into this to see if we can find a check that works.

thanks,
-- Shuah
Shuah Khan Sept. 28, 2024, 7:27 p.m. UTC | #9
On 9/28/24 11:54, Shuah Khan wrote:
> On 9/28/24 11:31, Guenter Roeck wrote:
>> On 9/27/24 17:08, Guenter Roeck wrote:
>>> On 9/27/24 13:45, Shuah Khan wrote:
>>>> On 9/27/24 10:19, Guenter Roeck wrote:
>>>>> Copying devicetree maintainers.
>>>>>
>>>>> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
>>>>>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
>>>>>>> Hi Stephen,
>>>>>>>
>>>>>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
>>>>>>>> Test that clks registered with 'struct clk_parent_data' work as
>>>>>>>> intended and can find their parents.
>>>>>>>>
>>>>>>>
>>>>>>> When testing this on arm64, I see the error below. The error is only
>>>>>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
>>>>>>> qemu parameter.
>>>>>>>
>>>>>>> Any idea what might cause the problem ?
>>>>>>>
>>>>>> I noticed that the new overlay tests fail as well, also with "path '/' not
>>>>>> found".
>>>>>>
>>>>>> [Maybe] answering my own question: I think the problem may be that there
>>>>>> is no devicetree file and thus no devicetree root when booting through
>>>>>> efi (in other words, of_root is NULL). Would it make sense to skip the
>>>>>> tests in that case ?
>>>>>>
>>>>>
>>>>> The problem is that of_root is not initialized in arm64 boots if ACPI
>>>>> is enabled.
>>>>>
>>>>>  From arch/arm64/kernel/setup.c:setup_arch():
>>>>>
>>>>>     if (acpi_disabled)
>>>>>         unflatten_device_tree();        // initializes of_root
>>>>>
>>>>> ACPI is enabled if the system boots from EFI. This also affects
>>>>> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
>>>>> fails the test if it doesn't.
>>>>>
>>>>> I think those tests need to add a check for this condition, or affected
>>>>> machines won't be able to run those unit tests. The obvious solution would
>>>>> be to check if of_root is set, but then the associated test case in
>>>>> CONFIG_OF_KUNIT_TEST would not make sense.
>>>>>
>>>>> Any suggestions ?
>>>>>
>>>>
>>>> Would it work if these tests check if acpi_disabled and skip if it isn't
>>>> disabled? It might be low overhead condition to check from these tests.
>>>>
>>>> acpi_disabled is exported:
>>>>
>>>> arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>>> arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>>> arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>>> arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled);
>>>>
>>>
>>> I don't think that would work. Looking through the use of acpi_init,
>>> I don't think that of_root is always NULL when acpi_init is false; that
>>> just happens to be the case on arm64 when booting through efi.
>>> However, even arm64 has the following code.
>>>
>>>          if (acpi_disabled)
>>>                  psci_dt_init();
>>>          else
>>>                  psci_acpi_init();
>>>
>>> While psci_dt_init() doesn't set of_root, it does try to do a devicetree
>>> match. So there must be some other condition where acpi_disabled is set
>>> but of_root is set anyway. I just have not found that code path.
>>>
>>
>> I ended up disabling all affected unit tests for arm64. I'll do the same
>> for other architectures if I encounter the problem there as well.
>>
>> Unfortunately that includes all clock unit tests because the tests requiring
>> devicetree support can not be enabled/disabled separately, but that can't be
>> helped and is still better than "mandatory" failures.
>>
> 

of_root is set in drivers/of/pdt.c when it creates the root node.
This could be a definitive test for kunit tests that depend on
devicetree support.

It is an exported symbol. drivers/of/base.c exports it.

thanks,
-- SHuah
Guenter Roeck Sept. 28, 2024, 9:32 p.m. UTC | #10
On 9/28/24 12:27, Shuah Khan wrote:
> On 9/28/24 11:54, Shuah Khan wrote:
>> On 9/28/24 11:31, Guenter Roeck wrote:
>>> On 9/27/24 17:08, Guenter Roeck wrote:
>>>> On 9/27/24 13:45, Shuah Khan wrote:
>>>>> On 9/27/24 10:19, Guenter Roeck wrote:
>>>>>> Copying devicetree maintainers.
>>>>>>
>>>>>> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
>>>>>>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
>>>>>>>> Hi Stephen,
>>>>>>>>
>>>>>>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
>>>>>>>>> Test that clks registered with 'struct clk_parent_data' work as
>>>>>>>>> intended and can find their parents.
>>>>>>>>>
>>>>>>>>
>>>>>>>> When testing this on arm64, I see the error below. The error is only
>>>>>>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
>>>>>>>> qemu parameter.
>>>>>>>>
>>>>>>>> Any idea what might cause the problem ?
>>>>>>>>
>>>>>>> I noticed that the new overlay tests fail as well, also with "path '/' not
>>>>>>> found".
>>>>>>>
>>>>>>> [Maybe] answering my own question: I think the problem may be that there
>>>>>>> is no devicetree file and thus no devicetree root when booting through
>>>>>>> efi (in other words, of_root is NULL). Would it make sense to skip the
>>>>>>> tests in that case ?
>>>>>>>
>>>>>>
>>>>>> The problem is that of_root is not initialized in arm64 boots if ACPI
>>>>>> is enabled.
>>>>>>
>>>>>>  From arch/arm64/kernel/setup.c:setup_arch():
>>>>>>
>>>>>>     if (acpi_disabled)
>>>>>>         unflatten_device_tree();        // initializes of_root
>>>>>>
>>>>>> ACPI is enabled if the system boots from EFI. This also affects
>>>>>> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
>>>>>> fails the test if it doesn't.
>>>>>>
>>>>>> I think those tests need to add a check for this condition, or affected
>>>>>> machines won't be able to run those unit tests. The obvious solution would
>>>>>> be to check if of_root is set, but then the associated test case in
>>>>>> CONFIG_OF_KUNIT_TEST would not make sense.
>>>>>>
>>>>>> Any suggestions ?
>>>>>>
>>>>>
>>>>> Would it work if these tests check if acpi_disabled and skip if it isn't
>>>>> disabled? It might be low overhead condition to check from these tests.
>>>>>
>>>>> acpi_disabled is exported:
>>>>>
>>>>> arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>>>> arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>>>> arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
>>>>> arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled);
>>>>>
>>>>
>>>> I don't think that would work. Looking through the use of acpi_init,
>>>> I don't think that of_root is always NULL when acpi_init is false; that
>>>> just happens to be the case on arm64 when booting through efi.
>>>> However, even arm64 has the following code.
>>>>
>>>>          if (acpi_disabled)
>>>>                  psci_dt_init();
>>>>          else
>>>>                  psci_acpi_init();
>>>>
>>>> While psci_dt_init() doesn't set of_root, it does try to do a devicetree
>>>> match. So there must be some other condition where acpi_disabled is set
>>>> but of_root is set anyway. I just have not found that code path.
>>>>
>>>
>>> I ended up disabling all affected unit tests for arm64. I'll do the same
>>> for other architectures if I encounter the problem there as well.
>>>
>>> Unfortunately that includes all clock unit tests because the tests requiring
>>> devicetree support can not be enabled/disabled separately, but that can't be
>>> helped and is still better than "mandatory" failures.
>>>
>>
> 
> of_root is set in drivers/of/pdt.c when it creates the root node.
> This could be a definitive test for kunit tests that depend on
> devicetree support.
> 

That is not always the case, including arm64. It is primarily set in
unflatten_devicetree(), which is not called on arm64 unless acpi_is disabled
(see above).

> It is an exported symbol. drivers/of/base.c exports it.
> 

Yes, checking if of_root is NULL and skipping the test in that case might help,
but then there is the of_dtb_root_node_populates_of_root unit test which
explicitly fails if of_root is NULL. The comment describing the test is

/*
  * Test that the 'of_root' global variable is always populated when DT code is
  * enabled. Remove this test once of_root is removed from global access.
  */

The devicetree unit test code explicitly assumes that of_root is set if
CONFIG_OF_EARLY_FLATTREE is enabled, but that is not always the case
(again, of_root is NULL on arm64 unless acpi is disabled).

Guenter
Stephen Boyd Oct. 3, 2024, 11:46 p.m. UTC | #11
Quoting Guenter Roeck (2024-09-28 14:32:35)
> On 9/28/24 12:27, Shuah Khan wrote:
> > On 9/28/24 11:54, Shuah Khan wrote:
> >> On 9/28/24 11:31, Guenter Roeck wrote:
> >>> On 9/27/24 17:08, Guenter Roeck wrote:
> >>>> On 9/27/24 13:45, Shuah Khan wrote:
> >>>>> On 9/27/24 10:19, Guenter Roeck wrote:
> >>>>>> Copying devicetree maintainers.
> >>>>>>
> >>>>>> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
> >>>>>>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
> >>>>>>>> Hi Stephen,
> >>>>>>>>
> >>>>>>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
> >>>>>>>>> Test that clks registered with 'struct clk_parent_data' work as
> >>>>>>>>> intended and can find their parents.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> When testing this on arm64, I see the error below. The error is only
> >>>>>>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
> >>>>>>>> qemu parameter.
> >>>>>>>>
> >>>>>>>> Any idea what might cause the problem ?
> >>>>>>>>
> >>>>>>> I noticed that the new overlay tests fail as well, also with "path '/' not
> >>>>>>> found".
> >>>>>>>
> >>>>>>> [Maybe] answering my own question: I think the problem may be that there
> >>>>>>> is no devicetree file and thus no devicetree root when booting through
> >>>>>>> efi (in other words, of_root is NULL). Would it make sense to skip the
> >>>>>>> tests in that case ?
> >>>>>>>
> >>>>>>
> >>>>>> The problem is that of_root is not initialized in arm64 boots if ACPI
> >>>>>> is enabled.
> >>>>>>
> >>>>>>  From arch/arm64/kernel/setup.c:setup_arch():
> >>>>>>
> >>>>>>     if (acpi_disabled)
> >>>>>>         unflatten_device_tree();        // initializes of_root

Oof I forgot that Rob didn't apply the patch that let an empty root live
on ARM64 ACPI systems. See this thread[1] for all the details.

> >>>>>>
> >>>>>> ACPI is enabled if the system boots from EFI. This also affects
> >>>>>> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
> >>>>>> fails the test if it doesn't.
> >>>>>>
> >>>>>> I think those tests need to add a check for this condition, or affected
> >>>>>> machines won't be able to run those unit tests. The obvious solution would
> >>>>>> be to check if of_root is set, but then the associated test case in
> >>>>>> CONFIG_OF_KUNIT_TEST would not make sense.
> >>>>>>
> >>>>>> Any suggestions ?
> >>>>>>

I think that's the best we can do for now. Basically add a check like

	if (IS_ENABLED(CONFIG_ARM64) && acpi_disabled)
		kunit_skip(test, "ARM64 + ACPI rejects DT overlays");

to the overlay application function and the DT test.

> >>>>>
> >>>>> Would it work if these tests check if acpi_disabled and skip if it isn't
> >>>>> disabled? It might be low overhead condition to check from these tests.
> >>>>>
> >>>>> acpi_disabled is exported:
> >>>>>
> >>>>> arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> >>>>> arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> >>>>> arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> >>>>> arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled);
> >>>>>
> >>>>
> >>>> I don't think that would work. Looking through the use of acpi_init,
> >>>> I don't think that of_root is always NULL when acpi_init is false; that
> >>>> just happens to be the case on arm64 when booting through efi.
> >>>> However, even arm64 has the following code.
> >>>>
> >>>>          if (acpi_disabled)
> >>>>                  psci_dt_init();
> >>>>          else
> >>>>                  psci_acpi_init();
> >>>>
> >>>> While psci_dt_init() doesn't set of_root, it does try to do a devicetree
> >>>> match. So there must be some other condition where acpi_disabled is set
> >>>> but of_root is set anyway. I just have not found that code path.
> >>>>
> >>>
> >>> I ended up disabling all affected unit tests for arm64. I'll do the same
> >>> for other architectures if I encounter the problem there as well.
> >>>
> >>> Unfortunately that includes all clock unit tests because the tests requiring
> >>> devicetree support can not be enabled/disabled separately, but that can't be
> >>> helped and is still better than "mandatory" failures.
> >>>
> >>
> > 
> > of_root is set in drivers/of/pdt.c when it creates the root node.
> > This could be a definitive test for kunit tests that depend on
> > devicetree support.
> > 
> 
> That is not always the case, including arm64. It is primarily set in
> unflatten_devicetree(), which is not called on arm64 unless acpi_is disabled
> (see above).

> 
> > It is an exported symbol. drivers/of/base.c exports it.
> > 
> 
> Yes, checking if of_root is NULL and skipping the test in that case might help,
> but then there is the of_dtb_root_node_populates_of_root unit test which
> explicitly fails if of_root is NULL. The comment describing the test is
> 
> /*
>   * Test that the 'of_root' global variable is always populated when DT code is
>   * enabled. Remove this test once of_root is removed from global access.
>   */
> 
> The devicetree unit test code explicitly assumes that of_root is set if
> CONFIG_OF_EARLY_FLATTREE is enabled, but that is not always the case
> (again, of_root is NULL on arm64 unless acpi is disabled).
> 

That DT test has been there for a few releases. Is this the first time
those tests have been run on arm64+acpi? I didn't try after sending the
patches and forgot that the patch was dropped.

How are you running kunit tests? I installed the qemu-efi-aarch64 debian
package to get QEMU_EFI.fd but passing that to the kunit.py run command
with --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd" didn't
get me beyond the point that the EFI stub boots linux. I think the
serial console must not be working and thus the kunit wrapper waits for
something to show up but nothing ever does. I haven't dug any further
though, so maybe you have a working command.

Here's my command that isn't working:

./tools/testing/kunit/kunit.py run --arch=arm64 --kunitconfig=drivers/of --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd"	

[1] https://lore.kernel.org/all/20240217010557.2381548-6-sboyd@kernel.org/
Guenter Roeck Oct. 4, 2024, 12:25 a.m. UTC | #12
On 10/3/24 16:46, Stephen Boyd wrote:
[ ... ]
> That DT test has been there for a few releases. Is this the first time
> those tests have been run on arm64+acpi? I didn't try after sending the
> patches and forgot that the patch was dropped.
> 

Previously I had the affected tests disabled and never tracked down the problem.
Since the problem is now spreading to additional tests, I finally tracked it down,
that is all.

> How are you running kunit tests? I installed the qemu-efi-aarch64 debian
> package to get QEMU_EFI.fd but passing that to the kunit.py run command
> with --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd" didn't
> get me beyond the point that the EFI stub boots linux. I think the
> serial console must not be working and thus the kunit wrapper waits for
> something to show up but nothing ever does. I haven't dug any further
> though, so maybe you have a working command.
> 

I run all tests during boot, not from the command line. I also use the -bios
command but don't recall any issues with the console. I specify the
console on the qemu command line; depending on the qemu machine it is either
ttyS0 or ttyAMA0. The init script then finds and selects the active console.

> Here's my command that isn't working:
> 
> ./tools/testing/kunit/kunit.py run --arch=arm64 --kunitconfig=drivers/of --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd"	
> 

I can't really see what that command is actually doing ;-).

I'll just keep the affected tests disabled on arm64 for the time being.

Thanks,
Guenter
Stephen Boyd Oct. 4, 2024, 12:42 a.m. UTC | #13
Quoting Guenter Roeck (2024-10-03 17:25:37)
> On 10/3/24 16:46, Stephen Boyd wrote:
> [ ... ]
> > That DT test has been there for a few releases. Is this the first time
> > those tests have been run on arm64+acpi? I didn't try after sending the
> > patches and forgot that the patch was dropped.
> > 
> 
> Previously I had the affected tests disabled and never tracked down the problem.
> Since the problem is now spreading to additional tests, I finally tracked it down,
> that is all.

Ok great. Good to know this isn't a new problem. Thanks for tracking it
down.

> 
> > How are you running kunit tests? I installed the qemu-efi-aarch64 debian
> > package to get QEMU_EFI.fd but passing that to the kunit.py run command
> > with --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd" didn't
> > get me beyond the point that the EFI stub boots linux. I think the
> > serial console must not be working and thus the kunit wrapper waits for
> > something to show up but nothing ever does. I haven't dug any further
> > though, so maybe you have a working command.
> > 
> 
> I run all tests during boot, not from the command line. I also use the -bios
> command but don't recall any issues with the console. I specify the
> console on the qemu command line; depending on the qemu machine it is either
> ttyS0 or ttyAMA0. The init script then finds and selects the active console.

Can you please describe how you run the kunit test? And provide the qemu
command you run to boot arm64 with acpi?

> 
> > Here's my command that isn't working:
> > 
> > ./tools/testing/kunit/kunit.py run --arch=arm64 --kunitconfig=drivers/of --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd"  
> > 
> 
> I can't really see what that command is actually doing ;-).

It eventually runs this qemu command

qemu-system-aarch64 -nodefaults -m 1024 -kernel .kunit/arch/arm64/boot/Image.gz -append 'kunit.enable=1 console=ttyAMA0 kunit_shutdown=reboot' -no-reboot -nographic -serial stdio -machine virt -cpu max,pauth-impdef=on -bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd

I see that it fails because the architected timer isn't there after I
add an earlycon=pl011,0x9000000 to the kernel commandline. I suspect
passing a bios like this is incorrect, but I rarely run qemu manually so
I don't know what I'm doing wrong.

NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
rcu: srcu_init: Setting srcu_struct sizes based on contention.
timer_probe: no matching timers found
Kernel panic - not syncing: Unable to initialise architected timer.
CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.11.0-rc1-00261-g497f7c30f184 #203
Call trace:
 dump_backtrace+0x94/0xec
 show_stack+0x18/0x24
 dump_stack_lvl+0x38/0x90
 dump_stack+0x18/0x24
 panic+0x36c/0x380
 early_brk64+0x0/0xa8
 start_kernel+0x27c/0x558
 __primary_switched+0x80/0x88
---[ end Kernel panic - not syncing: Unable to initialise architected timer. ]---

> 
> I'll just keep the affected tests disabled on arm64 for the time being.

We should skip the tests on arm64+acpi, which is similar to disabling
but not exactly the same. There will likely be more DT overlay usage in
kunit and so that will lead to more test disabling. Skipping properly is
the better solution.
Guenter Roeck Oct. 4, 2024, 4:52 a.m. UTC | #14
On 10/3/24 17:42, Stephen Boyd wrote:
> Quoting Guenter Roeck (2024-10-03 17:25:37)
>> On 10/3/24 16:46, Stephen Boyd wrote:
>> [ ... ]
>>> That DT test has been there for a few releases. Is this the first time
>>> those tests have been run on arm64+acpi? I didn't try after sending the
>>> patches and forgot that the patch was dropped.
>>>
>>
>> Previously I had the affected tests disabled and never tracked down the problem.
>> Since the problem is now spreading to additional tests, I finally tracked it down,
>> that is all.
> 
> Ok great. Good to know this isn't a new problem. Thanks for tracking it
> down.
> 
>>
>>> How are you running kunit tests? I installed the qemu-efi-aarch64 debian
>>> package to get QEMU_EFI.fd but passing that to the kunit.py run command
>>> with --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd" didn't
>>> get me beyond the point that the EFI stub boots linux. I think the
>>> serial console must not be working and thus the kunit wrapper waits for
>>> something to show up but nothing ever does. I haven't dug any further
>>> though, so maybe you have a working command.
>>>
>>
>> I run all tests during boot, not from the command line. I also use the -bios
>> command but don't recall any issues with the console. I specify the
>> console on the qemu command line; depending on the qemu machine it is either
>> ttyS0 or ttyAMA0. The init script then finds and selects the active console.
> 
> Can you please describe how you run the kunit test? And provide the qemu
> command you run to boot arm64 with acpi?
> 

Example command line:

qemu-system-aarch64 -M virt -m 512 \
      -kernel arch/arm64/boot/Image -no-reboot -nographic \
      -snapshot \
      -bios /opt/buildbot/rootfs/arm64/../firmware/QEMU_EFI-aarch64.fd \
      -device virtio-blk-device,drive=d0 \
      -drive file=rootfs.ext2,if=none,id=d0,format=raw \
      -cpu cortex-a57 -serial stdio -monitor none -no-reboot \
      --append "kunit.stats_enabled=2 kunit.filter=speed>slow root=/dev/vda rootwait earlycon=pl011,0x9000000 console=ttyAMA0"

That works fine for me. Configuration is arm64 defconfig plus various
debug and kunit options. I built the efi image myself from sources.
The root file system is from buildroot with modified init script.
kunit tests are all built into the kernel and run during boot.

>>
>> I'll just keep the affected tests disabled on arm64 for the time being.
> 
> We should skip the tests on arm64+acpi, which is similar to disabling
> but not exactly the same. There will likely be more DT overlay usage in
> kunit and so that will lead to more test disabling. Skipping properly is
> the better solution.

Sure, but having those tests fail all the time doesn't help either.
I'll re-enable the tests if / when they are skipped.

Thanks,
Guenter
Stephen Boyd Oct. 8, 2024, 11:12 p.m. UTC | #15
Quoting Guenter Roeck (2024-10-03 21:52:09)
> On 10/3/24 17:42, Stephen Boyd wrote:
> > 
> > Can you please describe how you run the kunit test? And provide the qemu
> > command you run to boot arm64 with acpi?
> > 
> 
> Example command line:
> 
> qemu-system-aarch64 -M virt -m 512 \
>       -kernel arch/arm64/boot/Image -no-reboot -nographic \
>       -snapshot \
>       -bios /opt/buildbot/rootfs/arm64/../firmware/QEMU_EFI-aarch64.fd \
>       -device virtio-blk-device,drive=d0 \
>       -drive file=rootfs.ext2,if=none,id=d0,format=raw \
>       -cpu cortex-a57 -serial stdio -monitor none -no-reboot \
>       --append "kunit.stats_enabled=2 kunit.filter=speed>slow root=/dev/vda rootwait earlycon=pl011,0x9000000 console=ttyAMA0"
> 
> That works fine for me. Configuration is arm64 defconfig plus various
> debug and kunit options. I built the efi image myself from sources.
> The root file system is from buildroot with modified init script.
> kunit tests are all built into the kernel and run during boot.

Thanks. I figured out that I was missing enabling CONFIG_ACPI. Here's my
commandline

./tools/testing/kunit/kunit.py run --arch=arm64 \
	--kunitconfig=drivers/of \
	--qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd -smp 2" \
	--kconfig_add="CONFIG_ACPI=y" \
	--kernel_args="earlycon=pl011,0x9000000"

Now I can boot and reproduce the failure, but there's another problem.
ACPI disables itself when it fails to find tables.

 ACPI: Unable to load the System Description Tables

This calls disable_acpi() which sets acpi_disabled to 1. This happens
before the unit test runs, meaning we can't reliably use 'acpi_disabled'
as a method to skip.

The best I can come up with then is to test for a NULL of_root when
CONFIG_ARM64 and CONFIG_ACPI are enabled, because the tests
intentionally don't work when both those configs are enabled and the
'of_root' isn't populated. In all other cases the 'of_root' missing is a
bug. I'll probably make this into some sort of kunit helper function in
of_private.h and send it to DT maintainers.

---8<----
diff --git a/drivers/of/of_kunit_helpers.c b/drivers/of/of_kunit_helpers.c
index 287d6c91bb37..a1330e183230 100644
--- a/drivers/of/of_kunit_helpers.c
+++ b/drivers/of/of_kunit_helpers.c
@@ -36,6 +36,9 @@ int of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
 	int ret;
 	int *copy_id;
 
+	if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ACPI) && !of_root)
+		kunit_skip(test, "arm64+acpi rejects overlays");
+
 	copy_id = kunit_kmalloc(test, sizeof(*copy_id), GFP_KERNEL);
 	if (!copy_id)
 		return -ENOMEM;
diff --git a/drivers/of/of_test.c b/drivers/of/of_test.c
index c85a258bc6ae..6cca43bf8029 100644
--- a/drivers/of/of_test.c
+++ b/drivers/of/of_test.c
@@ -38,6 +38,8 @@ static int of_dtb_test_init(struct kunit *test)
 {
 	if (!IS_ENABLED(CONFIG_OF_EARLY_FLATTREE))
 		kunit_skip(test, "requires CONFIG_OF_EARLY_FLATTREE");
+	if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ACPI) && !of_root)
+		kunit_skip(test, "arm64+acpi doesn't populate a root node on ACPI systems");
 
 	return 0;
 }
diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c
index 19a292cdeee3..3e7ac97a6796 100644
--- a/drivers/of/overlay_test.c
+++ b/drivers/of/overlay_test.c
@@ -64,6 +64,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test)
 
 	if (!IS_ENABLED(CONFIG_OF_EARLY_FLATTREE))
 		kunit_skip(test, "requires CONFIG_OF_EARLY_FLATTREE for root node");
+	if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ACPI) && !of_root)
+		kunit_skip(test, "arm64+acpi rejects overlays");
 
 	kunit_init_test(&fake, "fake test", NULL);
 	KUNIT_ASSERT_EQ(test, fake.status, KUNIT_SUCCESS);
Guenter Roeck Oct. 8, 2024, 11:27 p.m. UTC | #16
On 10/8/24 16:12, Stephen Boyd wrote:
> Quoting Guenter Roeck (2024-10-03 21:52:09)
>> On 10/3/24 17:42, Stephen Boyd wrote:
>>>
>>> Can you please describe how you run the kunit test? And provide the qemu
>>> command you run to boot arm64 with acpi?
>>>
>>
>> Example command line:
>>
>> qemu-system-aarch64 -M virt -m 512 \
>>        -kernel arch/arm64/boot/Image -no-reboot -nographic \
>>        -snapshot \
>>        -bios /opt/buildbot/rootfs/arm64/../firmware/QEMU_EFI-aarch64.fd \
>>        -device virtio-blk-device,drive=d0 \
>>        -drive file=rootfs.ext2,if=none,id=d0,format=raw \
>>        -cpu cortex-a57 -serial stdio -monitor none -no-reboot \
>>        --append "kunit.stats_enabled=2 kunit.filter=speed>slow root=/dev/vda rootwait earlycon=pl011,0x9000000 console=ttyAMA0"
>>
>> That works fine for me. Configuration is arm64 defconfig plus various
>> debug and kunit options. I built the efi image myself from sources.
>> The root file system is from buildroot with modified init script.
>> kunit tests are all built into the kernel and run during boot.
> 
> Thanks. I figured out that I was missing enabling CONFIG_ACPI. Here's my
> commandline
> 
> ./tools/testing/kunit/kunit.py run --arch=arm64 \
> 	--kunitconfig=drivers/of \
> 	--qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd -smp 2" \
> 	--kconfig_add="CONFIG_ACPI=y" \
> 	--kernel_args="earlycon=pl011,0x9000000"
> 
> Now I can boot and reproduce the failure, but there's another problem.
> ACPI disables itself when it fails to find tables.
> 
>   ACPI: Unable to load the System Description Tables
> 
> This calls disable_acpi() which sets acpi_disabled to 1. This happens
> before the unit test runs, meaning we can't reliably use 'acpi_disabled'
> as a method to skip.
> 
> The best I can come up with then is to test for a NULL of_root when
> CONFIG_ARM64 and CONFIG_ACPI are enabled, because the tests
> intentionally don't work when both those configs are enabled and the
> 'of_root' isn't populated. In all other cases the 'of_root' missing is a
> bug. I'll probably make this into some sort of kunit helper function in
> of_private.h and send it to DT maintainers.

Sounds good. Thanks a lot for tracking this down.

That makes me wonder though why only arm64 has that restriction. Both
riscv and loongarch have ACPI enabled in their defconfig files but call
unflatten_device_tree() unconditionally.

Oh well ...

Thanks,
Guenter
Stephen Boyd Oct. 9, 2024, 7:07 p.m. UTC | #17
Quoting Guenter Roeck (2024-10-08 16:27:37)
> On 10/8/24 16:12, Stephen Boyd wrote:
> > 
> > The best I can come up with then is to test for a NULL of_root when
> > CONFIG_ARM64 and CONFIG_ACPI are enabled, because the tests
> > intentionally don't work when both those configs are enabled and the
> > 'of_root' isn't populated. In all other cases the 'of_root' missing is a
> > bug. I'll probably make this into some sort of kunit helper function in
> > of_private.h and send it to DT maintainers.
> 
> Sounds good. Thanks a lot for tracking this down.
> 
> That makes me wonder though why only arm64 has that restriction. Both
> riscv and loongarch have ACPI enabled in their defconfig files but call
> unflatten_device_tree() unconditionally.
> 
> Oh well ...

Some of the reason is described in the thread I linked earlier. In
particular, this email from Mark[1]. There's also more comments from
Mark on an earlier patchset[2]. Maybe arm64 will allow it later, and
then we'll be able to revert this skip patch.

[1] https://lore.kernel.org/all/Zd4dQpHO7em1ji67@FVFF77S0Q05N.cambridge.arm.com/
[2] https://lore.kernel.org/all/ZaZtbU9hre3YhZam@FVFF77S0Q05N/
diff mbox series

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index d8482e015c49..0e4819c1cfd2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -508,6 +508,8 @@  config CLK_KUNIT_TEST
 	tristate "Basic Clock Framework Kunit Tests" if !KUNIT_ALL_TESTS
 	depends on KUNIT
 	default KUNIT_ALL_TESTS
+	select OF_OVERLAY if OF
+	select DTC
 	help
 	  Kunit tests for the common clock framework.
 
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 217aa4d4d48c..dddc9d87955a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -2,7 +2,9 @@ 
 # common clock types
 obj-$(CONFIG_HAVE_CLK)		+= clk-devres.o clk-bulk.o clkdev.o
 obj-$(CONFIG_COMMON_CLK)	+= clk.o
-obj-$(CONFIG_CLK_KUNIT_TEST)	+= clk_test.o
+obj-$(CONFIG_CLK_KUNIT_TEST)	+= clk-test.o
+clk-test-y			:= clk_test.o \
+				   kunit_clk_parent_data_test.dtbo.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-divider.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
diff --git a/drivers/clk/clk_parent_data_test.h b/drivers/clk/clk_parent_data_test.h
new file mode 100644
index 000000000000..eedd53ae910d
--- /dev/null
+++ b/drivers/clk/clk_parent_data_test.h
@@ -0,0 +1,10 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _CLK_PARENT_DATA_TEST_H
+#define _CLK_PARENT_DATA_TEST_H
+
+#define CLK_PARENT_DATA_1MHZ_NAME	"1mhz_fixed_legacy"
+#define CLK_PARENT_DATA_PARENT1		"parent_fwname"
+#define CLK_PARENT_DATA_PARENT2		"50"
+#define CLK_PARENT_DATA_50MHZ_NAME	"50_clk"
+
+#endif
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 39e2b5ff4f51..c2127f46fb93 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -4,12 +4,19 @@ 
  */
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 
 /* Needed for clk_hw_get_clk() */
 #include "clk.h"
 
+#include <kunit/clk.h>
+#include <kunit/of.h>
+#include <kunit/platform_device.h>
 #include <kunit/test.h>
 
+#include "clk_parent_data_test.h"
+
 static const struct clk_ops empty_clk_ops = { };
 
 #define DUMMY_CLOCK_INIT_RATE	(42 * 1000 * 1000)
@@ -2659,6 +2666,448 @@  static struct kunit_suite clk_mux_no_reparent_test_suite = {
 	.test_cases = clk_mux_no_reparent_test_cases,
 };
 
+struct clk_register_clk_parent_data_test_case {
+	const char *desc;
+	struct clk_parent_data pdata;
+};
+
+static void
+clk_register_clk_parent_data_test_case_to_desc(
+		const struct clk_register_clk_parent_data_test_case *t, char *desc)
+{
+	strcpy(desc, t->desc);
+}
+
+static const struct clk_register_clk_parent_data_test_case
+clk_register_clk_parent_data_of_cases[] = {
+	{
+		/*
+		 * Test that a clk registered with a struct device_node can
+		 * find a parent based on struct clk_parent_data::index.
+		 */
+		.desc = "clk_parent_data_of_index_test",
+		.pdata.index = 0,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device_node can
+		 * find a parent based on struct clk_parent_data::fwname.
+		 */
+		.desc = "clk_parent_data_of_fwname_test",
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT1,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device_node can
+		 * find a parent based on struct clk_parent_data::name.
+		 */
+		.desc = "clk_parent_data_of_name_test",
+		/* The index must be negative to indicate firmware not used */
+		.pdata.index = -1,
+		.pdata.name = CLK_PARENT_DATA_1MHZ_NAME,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device_node can
+		 * find a parent based on struct
+		 * clk_parent_data::{fw_name,name}.
+		 */
+		.desc = "clk_parent_data_of_fwname_name_test",
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT1,
+		.pdata.name = "not_matching",
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device_node can
+		 * find a parent based on struct clk_parent_data::{index,name}.
+		 * Index takes priority.
+		 */
+		.desc = "clk_parent_data_of_index_name_priority_test",
+		.pdata.index = 0,
+		.pdata.name = "not_matching",
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device_node can
+		 * find a parent based on struct
+		 * clk_parent_data::{index,fwname,name}. The fw_name takes
+		 * priority over index and name.
+		 */
+		.desc = "clk_parent_data_of_index_fwname_name_priority_test",
+		.pdata.index = 1,
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT1,
+		.pdata.name = "not_matching",
+	},
+};
+
+KUNIT_ARRAY_PARAM(clk_register_clk_parent_data_of_test, clk_register_clk_parent_data_of_cases,
+		  clk_register_clk_parent_data_test_case_to_desc)
+
+/**
+ * struct clk_register_clk_parent_data_of_ctx - Context for clk_parent_data OF tests
+ * @np: device node of clk under test
+ * @hw: clk_hw for clk under test
+ */
+struct clk_register_clk_parent_data_of_ctx {
+	struct device_node *np;
+	struct clk_hw hw;
+};
+
+static int clk_register_clk_parent_data_of_test_init(struct kunit *test)
+{
+	struct clk_register_clk_parent_data_of_ctx *ctx;
+
+	KUNIT_ASSERT_EQ(test, 0,
+			of_overlay_apply_kunit(test, kunit_clk_parent_data_test));
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	test->priv = ctx;
+
+	ctx->np = of_find_compatible_node(NULL, NULL, "test,clk-parent-data");
+	if (!ctx->np)
+		return -ENODEV;
+
+	of_node_put_kunit(test, ctx->np);
+
+	return 0;
+}
+
+/*
+ * Test that a clk registered with a struct device_node can find a parent based on
+ * struct clk_parent_data when the hw member isn't set.
+ */
+static void clk_register_clk_parent_data_of_test(struct kunit *test)
+{
+	struct clk_register_clk_parent_data_of_ctx *ctx = test->priv;
+	struct clk_hw *parent_hw;
+	const struct clk_register_clk_parent_data_test_case *test_param;
+	struct clk_init_data init = { };
+	struct clk *expected_parent, *actual_parent;
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->np);
+
+	expected_parent = of_clk_get_kunit(test, ctx->np, 0);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, expected_parent);
+
+	test_param = test->param_value;
+	init.parent_data = &test_param->pdata;
+	init.num_parents = 1;
+	init.name = "parent_data_of_test_clk";
+	init.ops = &clk_dummy_single_parent_ops;
+	ctx->hw.init = &init;
+	KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->np, &ctx->hw));
+
+	parent_hw = clk_hw_get_parent(&ctx->hw);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
+
+	actual_parent = clk_hw_get_clk_kunit(test, parent_hw, __func__);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, actual_parent);
+
+	KUNIT_EXPECT_TRUE(test, clk_is_match(expected_parent, actual_parent));
+}
+
+static struct kunit_case clk_register_clk_parent_data_of_test_cases[] = {
+	KUNIT_CASE_PARAM(clk_register_clk_parent_data_of_test,
+			 clk_register_clk_parent_data_of_test_gen_params),
+	{}
+};
+
+/*
+ * Test suite for registering clks with struct clk_parent_data and a struct
+ * device_node.
+ */
+static struct kunit_suite clk_register_clk_parent_data_of_suite = {
+	.name = "clk_register_clk_parent_data_of",
+	.init = clk_register_clk_parent_data_of_test_init,
+	.test_cases = clk_register_clk_parent_data_of_test_cases,
+};
+
+/**
+ * struct clk_register_clk_parent_data_device_ctx - Context for clk_parent_data device tests
+ * @dev: device of clk under test
+ * @hw: clk_hw for clk under test
+ * @pdrv: driver to attach to find @dev
+ */
+struct clk_register_clk_parent_data_device_ctx {
+	struct device *dev;
+	struct clk_hw hw;
+	struct platform_driver pdrv;
+};
+
+static inline struct clk_register_clk_parent_data_device_ctx *
+clk_register_clk_parent_data_driver_to_test_context(struct platform_device *pdev)
+{
+	return container_of(to_platform_driver(pdev->dev.driver),
+			    struct clk_register_clk_parent_data_device_ctx, pdrv);
+}
+
+static int clk_register_clk_parent_data_device_probe(struct platform_device *pdev)
+{
+	struct clk_register_clk_parent_data_device_ctx *ctx;
+
+	ctx = clk_register_clk_parent_data_driver_to_test_context(pdev);
+	ctx->dev = &pdev->dev;
+
+	return 0;
+}
+
+static void clk_register_clk_parent_data_device_driver(struct kunit *test)
+{
+	struct clk_register_clk_parent_data_device_ctx *ctx = test->priv;
+	static const struct of_device_id match_table[] = {
+		{ .compatible = "test,clk-parent-data" },
+		{ }
+	};
+
+	ctx->pdrv.probe = clk_register_clk_parent_data_device_probe;
+	ctx->pdrv.driver.of_match_table = match_table;
+	ctx->pdrv.driver.name = __func__;
+	ctx->pdrv.driver.owner = THIS_MODULE;
+
+	KUNIT_ASSERT_EQ(test, 0, kunit_platform_driver_register(test, &ctx->pdrv));
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dev);
+}
+
+static const struct clk_register_clk_parent_data_test_case
+clk_register_clk_parent_data_device_cases[] = {
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::index.
+		 */
+		.desc = "clk_parent_data_device_index_test",
+		.pdata.index = 1,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::fwname.
+		 */
+		.desc = "clk_parent_data_device_fwname_test",
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::name.
+		 */
+		.desc = "clk_parent_data_device_name_test",
+		/* The index must be negative to indicate firmware not used */
+		.pdata.index = -1,
+		.pdata.name = CLK_PARENT_DATA_50MHZ_NAME,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::{fw_name,name}.
+		 */
+		.desc = "clk_parent_data_device_fwname_name_test",
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+		.pdata.name = "not_matching",
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::{index,name}. Index
+		 * takes priority.
+		 */
+		.desc = "clk_parent_data_device_index_name_priority_test",
+		.pdata.index = 1,
+		.pdata.name = "not_matching",
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::{index,fwname,name}.
+		 * The fw_name takes priority over index and name.
+		 */
+		.desc = "clk_parent_data_device_index_fwname_name_priority_test",
+		.pdata.index = 0,
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+		.pdata.name = "not_matching",
+	},
+};
+
+KUNIT_ARRAY_PARAM(clk_register_clk_parent_data_device_test,
+		  clk_register_clk_parent_data_device_cases,
+		  clk_register_clk_parent_data_test_case_to_desc)
+
+/*
+ * Test that a clk registered with a struct device can find a parent based on
+ * struct clk_parent_data when the hw member isn't set.
+ */
+static void clk_register_clk_parent_data_device_test(struct kunit *test)
+{
+	struct clk_register_clk_parent_data_device_ctx *ctx;
+	const struct clk_register_clk_parent_data_test_case *test_param;
+	struct clk_hw *parent_hw;
+	struct clk_init_data init = { };
+	struct clk *expected_parent, *actual_parent;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+	test->priv = ctx;
+
+	clk_register_clk_parent_data_device_driver(test);
+
+	expected_parent = clk_get_kunit(test, ctx->dev, "50");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, expected_parent);
+
+	test_param = test->param_value;
+	init.parent_data = &test_param->pdata;
+	init.num_parents = 1;
+	init.name = "parent_data_device_test_clk";
+	init.ops = &clk_dummy_single_parent_ops;
+	ctx->hw.init = &init;
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, ctx->dev, &ctx->hw));
+
+	parent_hw = clk_hw_get_parent(&ctx->hw);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
+
+	actual_parent = clk_hw_get_clk_kunit(test, parent_hw, __func__);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, actual_parent);
+
+	KUNIT_EXPECT_TRUE(test, clk_is_match(expected_parent, actual_parent));
+}
+
+static const struct clk_register_clk_parent_data_test_case
+clk_register_clk_parent_data_device_hw_cases[] = {
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::hw.
+		 */
+		.desc = "clk_parent_data_device_hw_index_test",
+		/* The index must be negative to indicate firmware not used */
+		.pdata.index = -1,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::hw when
+		 * struct clk_parent_data::fw_name is set.
+		 */
+		.desc = "clk_parent_data_device_hw_fwname_test",
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::hw when struct
+		 * clk_parent_data::name is set.
+		 */
+		.desc = "clk_parent_data_device_hw_name_test",
+		/* The index must be negative to indicate firmware not used */
+		.pdata.index = -1,
+		.pdata.name = CLK_PARENT_DATA_50MHZ_NAME,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::hw when struct
+		 * clk_parent_data::{fw_name,name} are set.
+		 */
+		.desc = "clk_parent_data_device_hw_fwname_name_test",
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+		.pdata.name = "not_matching",
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::hw when struct
+		 * clk_parent_data::index is set. The hw pointer takes
+		 * priority.
+		 */
+		.desc = "clk_parent_data_device_hw_index_priority_test",
+		.pdata.index = 0,
+	},
+	{
+		/*
+		 * Test that a clk registered with a struct device can find a
+		 * parent based on struct clk_parent_data::hw when
+		 * struct clk_parent_data::{index,fwname,name} are set.
+		 * The hw pointer takes priority over everything else.
+		 */
+		.desc = "clk_parent_data_device_hw_index_fwname_name_priority_test",
+		.pdata.index = 0,
+		.pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+		.pdata.name = "not_matching",
+	},
+};
+
+KUNIT_ARRAY_PARAM(clk_register_clk_parent_data_device_hw_test,
+		  clk_register_clk_parent_data_device_hw_cases,
+		  clk_register_clk_parent_data_test_case_to_desc)
+
+/*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::hw.
+ */
+static void clk_register_clk_parent_data_device_hw_test(struct kunit *test)
+{
+	struct clk_register_clk_parent_data_device_ctx *ctx;
+	const struct clk_register_clk_parent_data_test_case *test_param;
+	struct clk_dummy_context *parent;
+	struct clk_hw *parent_hw;
+	struct clk_parent_data pdata = { };
+	struct clk_init_data init = { };
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+	test->priv = ctx;
+
+	clk_register_clk_parent_data_device_driver(test);
+
+	parent = kunit_kzalloc(test, sizeof(*parent), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	parent_hw = &parent->hw;
+	parent_hw->init = CLK_HW_INIT_NO_PARENT("parent-clk",
+						&clk_dummy_rate_ops, 0);
+
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, ctx->dev, parent_hw));
+
+	test_param = test->param_value;
+	memcpy(&pdata, &test_param->pdata, sizeof(pdata));
+	pdata.hw = parent_hw;
+	init.parent_data = &pdata;
+	init.num_parents = 1;
+	init.ops = &clk_dummy_single_parent_ops;
+	init.name = "parent_data_device_hw_test_clk";
+	ctx->hw.init = &init;
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, ctx->dev, &ctx->hw));
+
+	KUNIT_EXPECT_PTR_EQ(test, parent_hw, clk_hw_get_parent(&ctx->hw));
+}
+
+static struct kunit_case clk_register_clk_parent_data_device_test_cases[] = {
+	KUNIT_CASE_PARAM(clk_register_clk_parent_data_device_test,
+			 clk_register_clk_parent_data_device_test_gen_params),
+	KUNIT_CASE_PARAM(clk_register_clk_parent_data_device_hw_test,
+			 clk_register_clk_parent_data_device_hw_test_gen_params),
+	{}
+};
+
+static int clk_register_clk_parent_data_device_init(struct kunit *test)
+{
+	KUNIT_ASSERT_EQ(test, 0,
+			of_overlay_apply_kunit(test, kunit_clk_parent_data_test));
+
+	return 0;
+}
+
+/*
+ * Test suite for registering clks with struct clk_parent_data and a struct
+ * device.
+ */
+static struct kunit_suite clk_register_clk_parent_data_device_suite = {
+	.name = "clk_register_clk_parent_data_device",
+	.init = clk_register_clk_parent_data_device_init,
+	.test_cases = clk_register_clk_parent_data_device_test_cases,
+};
+
 kunit_test_suites(
 	&clk_leaf_mux_set_rate_parent_test_suite,
 	&clk_test_suite,
@@ -2671,7 +3120,9 @@  kunit_test_suites(
 	&clk_range_test_suite,
 	&clk_range_maximize_test_suite,
 	&clk_range_minimize_test_suite,
+	&clk_register_clk_parent_data_of_suite,
+	&clk_register_clk_parent_data_device_suite,
 	&clk_single_parent_mux_test_suite,
-	&clk_uncached_test_suite
+	&clk_uncached_test_suite,
 );
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/clk/kunit_clk_parent_data_test.dtso b/drivers/clk/kunit_clk_parent_data_test.dtso
new file mode 100644
index 000000000000..7d3ed9a5a2e8
--- /dev/null
+++ b/drivers/clk/kunit_clk_parent_data_test.dtso
@@ -0,0 +1,28 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+#include "clk_parent_data_test.h"
+
+&{/} {
+	fixed_50: kunit-clock-50MHz {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <50000000>;
+		clock-output-names = CLK_PARENT_DATA_50MHZ_NAME;
+	};
+
+	fixed_parent: kunit-clock-1MHz {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <1000000>;
+		clock-output-names = CLK_PARENT_DATA_1MHZ_NAME;
+	};
+
+	kunit-clock-controller {
+		compatible = "test,clk-parent-data";
+		clocks = <&fixed_parent>, <&fixed_50>;
+		clock-names = CLK_PARENT_DATA_PARENT1, CLK_PARENT_DATA_PARENT2;
+		#clock-cells = <1>;
+	};
+};