Skip to content

Conversation

VynDragon
Copy link
Contributor

@VynDragon VynDragon commented Aug 26, 2025

Adds BFLB DMA.

@VynDragon VynDragon requested a review from nandojve August 27, 2025 11:56
@VynDragon VynDragon force-pushed the bflb_dma branch 2 times, most recently from 238c537 to 75399de Compare August 30, 2025 03:34
Copy link

github-actions bot commented Aug 30, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_bouffalolab zephyrproject-rtos/hal_bouffalolab@9f2ab1b (main) zephyrproject-rtos/hal_bouffalolab#5 zephyrproject-rtos/hal_bouffalolab#5/files

DNM label due to: 1 project with PR revision

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

@github-actions github-actions bot added manifest manifest-hal_bouffalolab DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Aug 30, 2025
@VynDragon VynDragon marked this pull request as ready for review August 30, 2025 04:03
@zephyrbot zephyrbot added platform: Bouffalo Lab area: DMA Direct Memory Access area: RISCV RISCV Architecture (32-bit & 64-bit) area: Clock Control labels Aug 30, 2025
@VynDragon VynDragon force-pushed the bflb_dma branch 2 times, most recently from 776df91 to 2b5793b Compare September 1, 2025 17:52
uint32_t control = 0;
uint16_t size;

if (channel >= BFLB_DMA_CH_NB) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want, you can make this code misra compatible.
Add a while(0) { block at beggining
} close before the inval.

then, you must use break; to jump out without a goto;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you mean while(1) or while (true)? That is very cursed. Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is really while (0). I questioned this too first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that doesnt work... It just doesnt run the code in the while loop with while(0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I googled it, i think it's supposed to be a do while loop, not a while loop

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want, you can make this code misra compatible. Add a while(0) { block at beggining } close before the inval.

then, you must use break; to jump out without a goto;

Is this really a misra guidline? This makes things far more unreadable than a simple goto or early return. Most of zephyr uses goto out OR early return.

Would prefer to stick with Zephyr code styling over misra here unless you can point at the rules for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn’t resolved

{
const struct dma_bflb_config *cfg = dev->config;
uint32_t config, control;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless space, fix all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still have useless space

Comment on lines 381 to 348
}
stat->pending_length = (control & DMA_TRANSFERSIZE_MASK) >> DMA_TRANSFERSIZE_SHIFT;

stat->pending_length *= dma_bflb_get_transfer_size(dev, channel);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should add 1 space after the if block
then drop the space between the stat->pending_length
having the stat->pending_length glued with the if statement is OK and shows that you are preparing the data to be evaluated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@VynDragon
Copy link
Contributor Author

I think I have addressed all review comments, the do while loop has been reverted and simplified due to the lack of need for irq locking.

@VynDragon
Copy link
Contributor Author

I dont understand the build failures, why did enabling DMA enable flash tests? Also beagle board failing...

@josuah
Copy link
Contributor

josuah commented Oct 10, 2025

It is also failing on main.

1a5bef6 from #96749 enables flash tests for all boards with storage_partitions, assuming they all have a flash controller.

-  drivers.flash.common.disable_spi_nor:
-    filter: dt_compat_enabled("soc-nv-flash") and dt_compat_enabled("jedec,spi-nor")
+  drivers.flash.common.test_storage_partition:
+    filter: dt_label_with_parent_compat_enabled("storage_partition", "fixed-partitions")

For bouffalolab, the devicetree flashctrl node is there, but not the associated driver yet, as it comes in this PR

So merging it first will fix this current PR, or a hotfix PR to exclude the buggy board will also work.

@VynDragon
Copy link
Contributor Author

VynDragon commented Oct 10, 2025

enables flash tests for all boards with storage_partitions, assuming they all have a flash controller.

??? That's not very smart and means it shouldnt have passed the CI?

@VynDragon
Copy link
Contributor Author

And pretty obviously it should be checking for the presence of the 'flash' tag in supported

@josuah
Copy link
Contributor

josuah commented Oct 10, 2025

Yes CI should have been catching that.

I think twister does some heuristics to try to detect what might be affected by a PR to not re-test everything every time. Maybe that's why.

@VynDragon
Copy link
Contributor Author

VynDragon commented Oct 11, 2025

I also looked at the test file and it really makes no sense, best I can tell the flash tag is necessary to run any of those tests anyway, i'm not sure how this is getting them triggered for the bflb boards. Possibly using the filter: entry disables the main tag filter?

@VynDragon VynDragon modified the milestones: 4.3, v4.3.0 Oct 11, 2025
@zephyrbot zephyrbot added area: Tests Issues related to a particular existing or missing test area: Flash labels Oct 11, 2025
will be deleted

Signed-off-by: Camille BAUD <[email protected]>
Adds the DMA node

Signed-off-by: Camille BAUD <[email protected]>
Introduce BFLB's DMA.

Signed-off-by: Camille BAUD <[email protected]>
Guarantees De-gating the peripherals.

Signed-off-by: Camille BAUD <[email protected]>
Adds the dma to supported for testing

Signed-off-by: Camille BAUD <[email protected]>
@VynDragon VynDragon removed area: Flash area: Tests Issues related to a particular existing or missing test labels Oct 12, 2025
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants