[nrf toup] Factory data partition location change #499

kg-nordicsemi · 2024-10-23T08:33:34Z

factory data can be placed before or after settings partition
-added static_asserts, which checks whether factory data overlaps with settings partition
-added Kconfig which allow to choose the location of settings and factory data partition

kkasperczyk-no · 2024-10-23T08:54:11Z

src/platform/nrfconnect/FactoryDataProvider.h

@@ -76,11 +76,26 @@ struct InternalFlashFactoryData
        // and make sure we do not overlap with settings partition
        constexpr size_t kFactoryDataBlockEnd =
            (FACTORY_DATA_ADDRESS + FACTORY_DATA_SIZE + CONFIG_FPROTECT_BLOCK_SIZE - 1) & (-CONFIG_FPROTECT_BLOCK_SIZE);
+
+#ifdef CONFIG_CHIP_FACTORY_DATA_BEFORE_SETTINGS


It was not quite goal of this task to create an additional option that allows to put factory data before or after the settings. It's more about accepting both cases without setting any Kconfig option that determines it. I would say you can just check which address is bigger settings or factory data. And based on that you already know which partition is the first one. Then you just need to verify if the first partition + its size ends before the second partition.

ArekBalysNordic

Be aware of this note: https://github.com/nrfconnect/sdk-connectedhomeip/pull/499/files#diff-4521c3129b9a1ff0496c09d5af49e450c8e50b97a538845bcbae1d386f060536R64

kkasperczyk-no · 2024-10-24T05:47:52Z

src/platform/nrfconnect/FactoryDataProvider.h

        // and make sure we do not overlap with settings partition
        constexpr size_t kFactoryDataBlockEnd =
            (FACTORY_DATA_ADDRESS + FACTORY_DATA_SIZE + CONFIG_FPROTECT_BLOCK_SIZE - 1) & (-CONFIG_FPROTECT_BLOCK_SIZE);
-        static_assert(kFactoryDataBlockEnd <= PM_SETTINGS_STORAGE_ADDRESS,
+
+        constexpr size_t SettingsBlockEnd =


@ArekBalysNordic please correct me if I'm wrong, but I think that the settings partition does not need to be aligned to the fprotect block size, so the end address is just start address + size, right?

That's right. Only the partition that is protected by fprotect must be aligned to fprotect block size. So we need to take care of factory data only. We need to be aware to not overlap settings partition by fprotect. Keep in mind that the protect block size varies depending on our platform.

kkasperczyk-no · 2024-10-24T05:56:15Z

src/platform/nrfconnect/FactoryDataProvider.h

+        constexpr size_t kFactoryDataBlockBegin = FACTORY_DATA_ADDRESS & (-CONFIG_FPROTECT_BLOCK_SIZE);
+
+        constexpr bool overlapsCheck =
+            (SettingsBlockEnd <= kFactoryDataBlockBegin) || (kFactoryDataBlockEnd <= PM_SETTINGS_STORAGE_ADDRESS);


I'm not sure that this is correct. The problem that we consider here is the fact that fprotect has some fixed block size and we want to use it only on factory data (not on settings). There are two issues possible:

The factory data is before the settings and its size is not aligned to the fprotect block size, so bigger range than expected will be covered by fprotect. In such case we have to check if factory data start+ factory data size + alignment to fprotect block size will not overlap with the start address of settings.

The settings are before the factory data and the factory data size is not aligned. If the factory data is at the end of memory, the fprotect will do the alignment using the space before the factory data (because after that there is no memory left). In such case we have to check if memory end - (factory data size + alignment) does not overlap with the settings end.

ArekBalysNordic · 2024-10-23T14:36:48Z

src/platform/nrfconnect/FactoryDataProvider.h

        // and make sure we do not overlap with settings partition
        constexpr size_t kFactoryDataBlockEnd =
            (FACTORY_DATA_ADDRESS + FACTORY_DATA_SIZE + CONFIG_FPROTECT_BLOCK_SIZE - 1) & (-CONFIG_FPROTECT_BLOCK_SIZE);
-        static_assert(kFactoryDataBlockEnd <= PM_SETTINGS_STORAGE_ADDRESS,
+
+        constexpr size_t SettingsBlockEnd =


Suggested change

constexpr size_t SettingsBlockEnd =

constexpr size_t kSettingsBlockEnd =

replace its occurences.

ArekBalysNordic · 2024-10-23T14:37:32Z

src/platform/nrfconnect/FactoryDataProvider.h

+
+        constexpr size_t kFactoryDataBlockBegin = FACTORY_DATA_ADDRESS & (-CONFIG_FPROTECT_BLOCK_SIZE);
+
+        constexpr bool overlapsCheck =


Suggested change

constexpr bool overlapsCheck =

constexpr bool kOverlapsCheck =

& ditto

ArekBalysNordic · 2024-10-24T07:54:53Z

src/platform/nrfconnect/FactoryDataProvider.h

        // and make sure we do not overlap with settings partition
        constexpr size_t kFactoryDataBlockEnd =
            (FACTORY_DATA_ADDRESS + FACTORY_DATA_SIZE + CONFIG_FPROTECT_BLOCK_SIZE - 1) & (-CONFIG_FPROTECT_BLOCK_SIZE);
-        static_assert(kFactoryDataBlockEnd <= PM_SETTINGS_STORAGE_ADDRESS,
+
+        constexpr size_t SettingsBlockEnd =


That's right. Only the partition that is protected by fprotect must be aligned to fprotect block size. So we need to take care of factory data only. We need to be aware to not overlap settings partition by fprotect. Keep in mind that the protect block size varies depending on our platform.

kkasperczyk-no

LGTM overall, please consult with @ArekBalysNordic that alignment is correct.

kkasperczyk-no · 2024-10-29T07:44:37Z

src/platform/nrfconnect/FactoryDataProvider.h

@@ -76,11 +76,21 @@ struct InternalFlashFactoryData
        // and make sure we do not overlap with settings partition
        constexpr size_t kFactoryDataBlockEnd =
            (FACTORY_DATA_ADDRESS + FACTORY_DATA_SIZE + CONFIG_FPROTECT_BLOCK_SIZE - 1) & (-CONFIG_FPROTECT_BLOCK_SIZE);
-        static_assert(kFactoryDataBlockEnd <= PM_SETTINGS_STORAGE_ADDRESS,
+
+        constexpr size_t kFactoryDataBlockBegin = FACTORY_DATA_ADDRESS & (-CONFIG_FPROTECT_BLOCK_SIZE);


@ArekBalysNordic could you verify the alignment of the factory data beginning? TBH I don't know where this magic with & and - fprotect block size comes from.

ArekBalysNordic · 2024-10-29T08:20:55Z

src/platform/nrfconnect/FactoryDataProvider.h

@@ -76,11 +76,21 @@ struct InternalFlashFactoryData
        // and make sure we do not overlap with settings partition
        constexpr size_t kFactoryDataBlockEnd =
            (FACTORY_DATA_ADDRESS + FACTORY_DATA_SIZE + CONFIG_FPROTECT_BLOCK_SIZE - 1) & (-CONFIG_FPROTECT_BLOCK_SIZE);
-        static_assert(kFactoryDataBlockEnd <= PM_SETTINGS_STORAGE_ADDRESS,
+
+        constexpr size_t kFactoryDataBlockBegin = FACTORY_DATA_ADDRESS & (-CONFIG_FPROTECT_BLOCK_SIZE);


Why do you again calculate FcatoryDataBlockBegin since it is done above in function constexpr size_t FactoryDataBlockBegin() ?

Because if I do this like this: constexpr size_t kFactoryDataBlockBegin = FactoryDataBlockBegin();
there is an following error:

src/platform/nrfconnect/FactoryDataProvider.h

ArekBalysNordic

Please rebase the related sdk-nrf manifest PR to see whether it works before merging.

- factory data can be placed before or after settings partition Signed-off-by: Konrad Grucel <[email protected]>

github-actions bot added config platform nrf connect labels Oct 23, 2024

NordicBuilder mentioned this pull request Oct 23, 2024

manifest: sdk-connectedhomeip: Update revision nrfconnect/sdk-nrf#18114

Open

kg-nordicsemi requested review from kkasperczyk-no and ArekBalysNordic October 23, 2024 08:36

kkasperczyk-no reviewed Oct 23, 2024

View reviewed changes

ArekBalysNordic reviewed Oct 23, 2024

View reviewed changes

kg-nordicsemi force-pushed the Factory-data-before-settings branch 2 times, most recently from a4c701d to a1c3d8c Compare October 23, 2024 13:44

kkasperczyk-no reviewed Oct 24, 2024

View reviewed changes

ArekBalysNordic reviewed Oct 24, 2024

View reviewed changes

kg-nordicsemi force-pushed the Factory-data-before-settings branch from a1c3d8c to e7de8f7 Compare October 24, 2024 13:01

kg-nordicsemi requested review from kkasperczyk-no and ArekBalysNordic October 24, 2024 13:56

kkasperczyk-no approved these changes Oct 29, 2024

View reviewed changes

ArekBalysNordic reviewed Oct 29, 2024

View reviewed changes

kg-nordicsemi force-pushed the Factory-data-before-settings branch from e7de8f7 to 7afc8b5 Compare November 5, 2024 07:53

ArekBalysNordic approved these changes Nov 21, 2024

View reviewed changes

[nrf toup] Factory data partition location change

6b6c2eb

- factory data can be placed before or after settings partition Signed-off-by: Konrad Grucel <[email protected]>

kg-nordicsemi force-pushed the Factory-data-before-settings branch from 7afc8b5 to 6b6c2eb Compare November 25, 2024 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nrf toup] Factory data partition location change #499

[nrf toup] Factory data partition location change #499

kg-nordicsemi commented Oct 23, 2024

kkasperczyk-no Oct 23, 2024 •

edited

Loading

ArekBalysNordic left a comment

kkasperczyk-no Oct 24, 2024

ArekBalysNordic Oct 24, 2024 •

edited

Loading

kkasperczyk-no Oct 24, 2024 •

edited

Loading

ArekBalysNordic Oct 23, 2024

ArekBalysNordic Oct 23, 2024

ArekBalysNordic Oct 23, 2024

ArekBalysNordic Oct 24, 2024 •

edited

Loading

kkasperczyk-no left a comment

kkasperczyk-no Oct 29, 2024

ArekBalysNordic Oct 29, 2024

kg-nordicsemi Oct 30, 2024

ArekBalysNordic left a comment

	constexpr size_t SettingsBlockEnd =
	constexpr size_t kSettingsBlockEnd =


		constexpr size_t kFactoryDataBlockBegin = FACTORY_DATA_ADDRESS & (-CONFIG_FPROTECT_BLOCK_SIZE);

		constexpr bool overlapsCheck =

	constexpr bool overlapsCheck =
	constexpr bool kOverlapsCheck =

[nrf toup] Factory data partition location change #499

Are you sure you want to change the base?

[nrf toup] Factory data partition location change #499

Conversation

kg-nordicsemi commented Oct 23, 2024

kkasperczyk-no Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

ArekBalysNordic left a comment

Choose a reason for hiding this comment

kkasperczyk-no Oct 24, 2024

Choose a reason for hiding this comment

ArekBalysNordic Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

kkasperczyk-no Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

ArekBalysNordic Oct 23, 2024

Choose a reason for hiding this comment

ArekBalysNordic Oct 23, 2024

Choose a reason for hiding this comment

ArekBalysNordic Oct 23, 2024

Choose a reason for hiding this comment

ArekBalysNordic Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

kkasperczyk-no left a comment

Choose a reason for hiding this comment

kkasperczyk-no Oct 29, 2024

Choose a reason for hiding this comment

ArekBalysNordic Oct 29, 2024

Choose a reason for hiding this comment

kg-nordicsemi Oct 30, 2024

Choose a reason for hiding this comment

ArekBalysNordic left a comment

Choose a reason for hiding this comment

kkasperczyk-no Oct 23, 2024 •

edited

Loading

ArekBalysNordic Oct 24, 2024 •

edited

Loading

kkasperczyk-no Oct 24, 2024 •

edited

Loading

ArekBalysNordic Oct 24, 2024 •

edited

Loading