fat_fs: Add support for the "win" variable alignment in FATFS struct #41096

ngboonkhai · 2021-12-12T17:54:06Z

This is to support Sypnosys Designware SDMMC controller read and write
operation where the buffer address is needed to be 16bytes and 512bytes
aligned. Adding macro FS_FATFS_WINDOW_ALIGNMENT to align the "win" variable
address in FATFS struct.

Signed-off-by: Boon Khai Ng [email protected]

de-nordic

The change requires west manifest update that should be part of the PR.

ngboonkhai · 2021-12-13T11:45:32Z

The change requires west manifest update that should be part of the PR.

Hi Ermel,

Thanks for reviewing my PR, should I go and update the west manifest? If yes, can point me the documentation on what should I update on the west manifest. Thanks.

Regards,
Boon Khai

de-nordic · 2021-12-13T12:31:36Z

Thanks for reviewing my PR, should I go and update the west manifest? If yes, can point me the documentation on what should I update on the west manifest. Thanks.

This change applies only when certain changes to FAT FS are applied (zephyrproject-rtos/fatfs#9), so what you need is to have commit that would propose the change to the FAT FS for by providing modified west.yml that would refer to the PR you want to introduce and then the commit that adds Zephyr changes on top.

This is my PR #36302, as an example, that has been introducing new version of FAT FS into Zephyr; there are two commits: the first one introduces new version of FAT FS by changing its manifest sha and the other commit adds some supporting code. This is very similar to your case here.
The difference is that you need to start with west.yml that, instead of containing sha, will refer to your pull request to fatfs PR as: pull/9/head

ngboonkhai · 2021-12-13T13:44:40Z

Thanks for reviewing my PR, should I go and update the west manifest? If yes, can point me the documentation on what should I update on the west manifest. Thanks.

This change applies only when certain changes to FAT FS are applied (zephyrproject-rtos/fatfs#9), so what you need is to have commit that would propose the change to the FAT FS for by providing modified west.yml that would refer to the PR you want to introduce and then the commit that adds Zephyr changes on top.

This is my PR #36302, as an example, that has been introducing new version of FAT FS into Zephyr; there are two commits: the first one introduces new version of FAT FS by changing its manifest sha and the other commit adds some supporting code. This is very similar to your case here. The difference is that you need to start with west.yml that, instead of containing sha, will refer to your pull request to fatfs PR as: pull/9/head

Hi Ermel,

Ah, okay got it.

So for the PR in (zephyrproject-rtos/fatfs#9) I still need to retain right?
then for this PR, I will need to resubmit with a force push with a updated west manifest?

Regards,
Boon Khai

de-nordic · 2021-12-13T14:45:39Z

So for the PR in (zephyrproject-rtos/fatfs#9) I still need to retain right? then for this PR, I will need to resubmit with a force push with a updated west manifest?

Yes, you need both PRs because this PR, the one we are talking under, is actually attempting to introduce, into Zephyr, changes that you have done to the other module via the zephyrproject-rtos/fatfs#9.
This allows anyone to use west command with your PR to download and test your solution.
When you will have the zephyrproject-rtos/fatfs#9 accepted, then once again you will have to update this PR with the sha that your merged changes will get on the FAT FS module tree.

ngboonkhai · 2021-12-14T03:34:08Z

So for the PR in (zephyrproject-rtos/fatfs#9) I still need to retain right? then for this PR, I will need to resubmit with a force push with a updated west manifest?

Yes, you need both PRs because this PR, the one we are talking under, is actually attempting to introduce, into Zephyr, changes that you have done to the other module via the zephyrproject-rtos/fatfs#9. This allows anyone to use west command with your PR to download and test your solution. When you will have the zephyrproject-rtos/fatfs#9 accepted, then once again you will have to update this PR with the sha that your merged changes will get on the FAT FS module tree.

Hi Ermel,

I found that there is no reviewer in the pull request zephyrproject-rtos/fatfs#9 . I cant find any button to add the reviewer, can you help to review my code at the PR as well?

Regards,
Boon Khai

nvlsianpu

@ngboonkhai You should add reference to the fatfs module PR in west.yaml patch.
Something like in https://github.com/zephyrproject-rtos/zephyr/pull/41262/files

ngboonkhai · 2021-12-16T16:53:41Z

@ngboonkhai You should add reference to the fatfs module PR in west.yaml patch. Something like in https://github.com/zephyrproject-rtos/zephyr/pull/41262/files

Hi nvlsianpu,

Thanks for remind, Ermel has told me that I need to update the west.yaml as well, was waiting for the PR at zephyrproject-rtos/fatfs#9.

After that I will force push the PR here with the hash with the updated west.yaml.

Wonder if you can help me the PR at zephyrproject-rtos/fatfs#9 ?

Regards,
Boon Khai

nvlsianpu · 2021-12-17T12:51:11Z

@ngboonkhai contribution process expects that you should first have PR like this one with pull/9/head reference for fatfs module - this allows us to see CI results. Then once ( with approvals on this PR and the fatfs module PR) fatfs PR will be merged that reference will needed to be changed to the SHA.

subsys/fs/Kconfig.fatfs

de-nordic · 2022-01-10T09:21:44Z

@ngboonkhai You should add reference to the fatfs module PR in west.yaml patch. Something like in https://github.com/zephyrproject-rtos/zephyr/pull/41262/files

Hi nvlsianpu,

Thanks for remind, Ermel has told me that I need to update the west.yaml as well, was waiting for the PR at zephyrproject-rtos/fatfs#9.

After that I will force push the PR here with the hash with the updated west.yaml.

Wonder if you can help me the PR at zephyrproject-rtos/fatfs#9 ?

Regards, Boon Khai

Can you modify the west.yml to pull in the FAT FS PR? Otherwise the sanity here is run with FAT FS that does not support the option you are adding.
Your PR here should consist of two commits:

the one that modifies the west.yml to include changes in the FAT FS pr;
the additions that make the changes from 1) available to FAT FS driver within Zephyr.

The FAT FS PR will not be merged till this PR is able to run with it included through west.
When sanity completes with the PR reference, we can merge the FAT FS PR and then you will have to, again, modify the west.yml to, this time, include sha of the FAT FS commit that would be including your change to FAT FS repo.
Then, if everything goes OK, we can merge this PR.

Btw. I did test builds of this change, after checking out all needed repos by hand (because west is not done...) and here is example of memory impact, for your information. with window set to 1 (default):

west build -d ttt -b nrf52840_blip zephyr/samples/subsys/fs/fat_fs/ -- -DCONFIG_FS_FATFS_WINDOW_ALIGNMENT=1
...
Memory region         Used Size  Region Size  %age Used
           FLASH:       47320 B         1 MB      4.51%
            SRAM:        8952 B       256 KB      3.41%
        IDT_LIST:          0 GB         2 KB      0.00%

and with window set to 512:

west build -d ttt -b nrf52840_blip zephyr/samples/subsys/fs/fat_fs/ -- -DCONFIG_FS_FATFS_WINDOW_ALIGNMENT=512
...
Memory region         Used Size  Region Size  %age Used
           FLASH:       47336 B         1 MB      4.51%
            SRAM:        9824 B       256 KB      3.75%
        IDT_LIST:          0 GB         2 KB      0.00%

ngboonkhai · 2022-01-10T15:02:34Z

@ngboonkhai You should add reference to the fatfs module PR in west.yaml patch. Something like in https://github.com/zephyrproject-rtos/zephyr/pull/41262/files

Hi nvlsianpu,
Thanks for remind, Ermel has told me that I need to update the west.yaml as well, was waiting for the PR at zephyrproject-rtos/fatfs#9.
After that I will force push the PR here with the hash with the updated west.yaml.
Wonder if you can help me the PR at zephyrproject-rtos/fatfs#9 ?
Regards, Boon Khai

Can you modify the west.yml to pull in the FAT FS PR? Otherwise the sanity here is run with FAT FS that does not support the option you are adding. Your PR here should consist of two commits:

the one that modifies the west.yml to include changes in the FAT FS pr;

the additions that make the changes from 1) available to FAT FS driver within Zephyr.

The FAT FS PR will not be merged till this PR is able to run with it included through west. When sanity completes with the PR reference, we can merge the FAT FS PR and then you will have to, again, modify the west.yml to, this time, include sha of the FAT FS commit that would be including your change to FAT FS repo. Then, if everything goes OK, we can merge this PR.

Btw. I did test builds of this change, after checking out all needed repos by hand (because west is not done...) and here is example of memory impact, for your information. with window set to 1 (default):
west build -d ttt -b nrf52840_blip zephyr/samples/subsys/fs/fat_fs/ -- -DCONFIG_FS_FATFS_WINDOW_ALIGNMENT=1
...
Memory region         Used Size  Region Size  %age Used
           FLASH:       47320 B         1 MB      4.51%
            SRAM:        8952 B       256 KB      3.41%
        IDT_LIST:          0 GB         2 KB      0.00%
and with window set to 512:
west build -d ttt -b nrf52840_blip zephyr/samples/subsys/fs/fat_fs/ -- -DCONFIG_FS_FATFS_WINDOW_ALIGNMENT=512
...
Memory region         Used Size  Region Size  %age Used
           FLASH:       47336 B         1 MB      4.51%
            SRAM:        9824 B       256 KB      3.75%
        IDT_LIST:          0 GB         2 KB      0.00%

Hi @de-nordic ,

Thanks for helping to run the compilation manually.

I don't quite understand on this part, if the PR in the fatfs not merge in yet, how do I get the revision id?
The only id can get is this 32f6befc3d4dda481f9649663e5122196d2b7c06. However when I try to clone the fatfs manually, i cant checkout to the commit id.

Regards,
Boon Khai

de-nordic · 2022-01-10T15:25:02Z

I don't quite understand on this part, if the PR in the fatfs not merge in yet, how do I get the revision id? The only id can get is this 32f6befc3d4dda481f9649663e5122196d2b7c06. However when I try to clone the fatfs manually, i cant checkout to the commit id.

You use the /pull/9/head as the sha:

...
    - name: fatfs
      revision: /pull/9/head
      path: modules/fs/fatfs
      groups:
...

This allows sanity, and interested reviewers, to get your complete solution and test it.

github-actions · 2022-01-11T02:31:27Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
fatfs	zephyrproject-rtos/fatfs@`09a9d91`	zephyrproject-rtos/fatfs@`9237454` (master)	zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

ngboonkhai · 2022-01-11T02:33:25Z

I don't quite understand on this part, if the PR in the fatfs not merge in yet, how do I get the revision id? The only id can get is this 32f6befc3d4dda481f9649663e5122196d2b7c06. However when I try to clone the fatfs manually, i cant checkout to the commit id.

You use the /pull/9/head as the sha:
...
    - name: fatfs
      revision: /pull/9/head
      path: modules/fs/fatfs
      groups:
...
This allows sanity, and interested reviewers, to get your complete solution and test it.

Hi @de-nordic ,

Okay, done added.
Thanks.

But after the review process, I need to force push again with the revision id that merged in into the fatfs?

Regards,
Boon Khai

ngboonkhai · 2022-01-11T16:10:23Z

But after the review process, I need to force push again with the revision id that merged in into the fatfs?

Yest, that will ave to happen.

Ah okay

ngboonkhai · 2022-01-11T16:10:41Z

The process is described here: https://docs.zephyrproject.org/latest/guides/modules.html#changes-to-existing-module

Okay, noted.

subsys/fs/Kconfig.fatfs

de-nordic

Change order of commits: west change goes in first.

In west commit remove pull/9/head from commit title and body, because the same data will be within source anyway, instead mention what the update actually brings in, as a rationale for the update; look at commit 284fbf0, as an example, where you will see that the commit message describes what the west update brings in with module.

ngboonkhai · 2022-01-14T05:30:10Z

Change order of commits: west change goes in first.

In west commit remove pull/9/head from commit title and body, because the same data will be within source anyway, instead mention what the update actually brings in, as a rationale for the update; look at commit 284fbf0, as an example, where you will see that the commit message describes what the west update brings in with module.

Hi Ermel,

Alright, done force push with the the above request

Regards,
Boon Khai

nvlsianpu · 2022-01-20T17:19:18Z

@ngboonkhai you can left behind west update now - it was bumped up by #41885

ngboonkhai · 2022-01-20T18:13:07Z

@ngboonkhai you can left behind west update now - it was bumped up by #41885

Hi @nvlsianpu,

Meaning i need to rebase my repo to the latest and force push?

Regards,
Boon Khai

nvlsianpu · 2022-01-20T20:28:38Z

yes

ngboonkhai · 2022-01-21T06:04:21Z

yes

@nvlsianpu how do I rerun the twister without force push? the twister failed, due to I update the zephyr mainline first before update the fatfs repo, and now I need to rerun the twister.

de-nordic · 2022-01-21T09:12:06Z

@ngboonkhai I have merged the FAT FS module, please update revision in west.yaml

Update west.yaml to the commit consist of the new config option CONFIG_FATFS_WINDOW_ALIGNMENT to align the win variable with specific alignment Signed-off-by: Boon Khai Ng <[email protected]>

subsys/fs/Kconfig.fatfs

This is to support Sypnosys Designware SDMMC controller read and write operation where the buffer address is needed to be 16bytes and 512bytes aligned. Adding macro FS_FATFS_WINDOW_ALIGNMENT to align the "win" variable address in FATFS struct. Signed-off-by: Boon Khai Ng <[email protected]>

de-nordic

Looks OK. Thanks!

ngboonkhai · 2022-02-07T02:18:36Z

Hi @de-nordic @nvlsianpu

Please help me to merge the changes.

Thanks.

de-nordic · 2022-02-07T08:25:33Z

Please help me to merge the changes.

Unfortunately you have to wait till code-freeze ends for the 3.0 release (https://github.com/zephyrproject-rtos/zephyr/wiki/Program-Management#actual-and-planned-milestone-dates), then we will be poking at people to merge it.

ngboonkhai requested a review from nashif as a code owner December 12, 2021 17:54

github-actions bot added the area: File System label Dec 12, 2021

de-nordic requested changes Dec 13, 2021

View reviewed changes

zephyrbot requested review from Laczen, nvlsianpu and vanwinkeljan December 13, 2021 17:45

zephyrbot assigned nvlsianpu Dec 13, 2021

Laczen removed their request for review December 13, 2021 19:36

nvlsianpu reviewed Dec 16, 2021

View reviewed changes

de-nordic reviewed Dec 23, 2021

View reviewed changes

subsys/fs/Kconfig.fatfs Outdated Show resolved Hide resolved

de-nordic reviewed Dec 23, 2021

View reviewed changes

subsys/fs/Kconfig.fatfs Outdated Show resolved Hide resolved

de-nordic reviewed Dec 30, 2021

View reviewed changes

subsys/fs/Kconfig.fatfs Outdated Show resolved Hide resolved

de-nordic mentioned this pull request Jan 7, 2022

fat_fs: Add support for the "win" variable alignment in FATFS struct zephyrproject-rtos/fatfs#9

Merged

ngboonkhai force-pushed the add-memory-alignment-option-fatfs branch from 582c279 to 4efd23c Compare January 10, 2022 02:45

github-actions bot added area: Modules manifest west manifest-fatfs labels Jan 11, 2022

ngboonkhai force-pushed the add-memory-alignment-option-fatfs branch from f4dde44 to 3cd5c1d Compare January 11, 2022 15:53

github-actions bot added the DNM This PR should not be merged (Do Not Merge) label Jan 11, 2022

de-nordic requested review from de-nordic and nvlsianpu January 12, 2022 09:50

de-nordic reviewed Jan 12, 2022

View reviewed changes

subsys/fs/Kconfig.fatfs Outdated Show resolved Hide resolved

de-nordic requested changes Jan 12, 2022

View reviewed changes

ngboonkhai force-pushed the add-memory-alignment-option-fatfs branch from 3cd5c1d to d9552bc Compare January 13, 2022 18:33

de-nordic self-requested a review January 14, 2022 11:11

ngboonkhai force-pushed the add-memory-alignment-option-fatfs branch from d9552bc to 2f1e8f2 Compare January 21, 2022 05:28

ngboonkhai force-pushed the add-memory-alignment-option-fatfs branch from 2f1e8f2 to cd407f0 Compare January 21, 2022 06:11

west.yml: update fatfs for new config option

983c445

Update west.yaml to the commit consist of the new config option CONFIG_FATFS_WINDOW_ALIGNMENT to align the win variable with specific alignment Signed-off-by: Boon Khai Ng <[email protected]>

ngboonkhai force-pushed the add-memory-alignment-option-fatfs branch from cd407f0 to 2241782 Compare January 21, 2022 16:43

github-actions bot removed the DNM This PR should not be merged (Do Not Merge) label Jan 21, 2022

de-nordic reviewed Jan 21, 2022

View reviewed changes

subsys/fs/Kconfig.fatfs Outdated Show resolved Hide resolved

ngboonkhai force-pushed the add-memory-alignment-option-fatfs branch from 2241782 to 772189a Compare January 21, 2022 17:40

de-nordic approved these changes Jan 21, 2022

View reviewed changes

nvlsianpu approved these changes Jan 24, 2022

View reviewed changes

nashif merged commit e46d8a1 into zephyrproject-rtos:main Feb 22, 2022

ngboonkhai deleted the add-memory-alignment-option-fatfs branch February 22, 2022 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fat_fs: Add support for the "win" variable alignment in FATFS struct #41096

fat_fs: Add support for the "win" variable alignment in FATFS struct #41096

ngboonkhai commented Dec 12, 2021 •

edited

Loading

de-nordic left a comment

ngboonkhai commented Dec 13, 2021

de-nordic commented Dec 13, 2021

ngboonkhai commented Dec 13, 2021

de-nordic commented Dec 13, 2021

ngboonkhai commented Dec 14, 2021

nvlsianpu left a comment

ngboonkhai commented Dec 16, 2021

nvlsianpu commented Dec 17, 2021

de-nordic commented Jan 10, 2022 •

edited

Loading

ngboonkhai commented Jan 10, 2022 •

edited

Loading

de-nordic commented Jan 10, 2022

github-actions bot commented Jan 11, 2022 •

edited

Loading

ngboonkhai commented Jan 11, 2022

ngboonkhai commented Jan 11, 2022

ngboonkhai commented Jan 11, 2022

de-nordic left a comment

ngboonkhai commented Jan 14, 2022

nvlsianpu commented Jan 20, 2022

ngboonkhai commented Jan 20, 2022

nvlsianpu commented Jan 20, 2022

ngboonkhai commented Jan 21, 2022

de-nordic commented Jan 21, 2022

de-nordic left a comment

ngboonkhai commented Feb 7, 2022

de-nordic commented Feb 7, 2022

fat_fs: Add support for the "win" variable alignment in FATFS struct #41096

fat_fs: Add support for the "win" variable alignment in FATFS struct #41096

Conversation

ngboonkhai commented Dec 12, 2021 • edited Loading

de-nordic left a comment

Choose a reason for hiding this comment

ngboonkhai commented Dec 13, 2021

de-nordic commented Dec 13, 2021

ngboonkhai commented Dec 13, 2021

de-nordic commented Dec 13, 2021

ngboonkhai commented Dec 14, 2021

nvlsianpu left a comment

Choose a reason for hiding this comment

ngboonkhai commented Dec 16, 2021

nvlsianpu commented Dec 17, 2021

de-nordic commented Jan 10, 2022 • edited Loading

ngboonkhai commented Jan 10, 2022 • edited Loading

de-nordic commented Jan 10, 2022

github-actions bot commented Jan 11, 2022 • edited Loading

ngboonkhai commented Jan 11, 2022

ngboonkhai commented Jan 11, 2022

ngboonkhai commented Jan 11, 2022

de-nordic left a comment

Choose a reason for hiding this comment

ngboonkhai commented Jan 14, 2022

nvlsianpu commented Jan 20, 2022

ngboonkhai commented Jan 20, 2022

nvlsianpu commented Jan 20, 2022

ngboonkhai commented Jan 21, 2022

de-nordic commented Jan 21, 2022

de-nordic left a comment

Choose a reason for hiding this comment

ngboonkhai commented Feb 7, 2022

de-nordic commented Feb 7, 2022

ngboonkhai commented Dec 12, 2021 •

edited

Loading

de-nordic commented Jan 10, 2022 •

edited

Loading

ngboonkhai commented Jan 10, 2022 •

edited

Loading

github-actions bot commented Jan 11, 2022 •

edited

Loading