Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/imx-enet: Add support for ethernet in imxrt1064-evk #96

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

julianuziemblo
Copy link

@julianuziemblo julianuziemblo commented Aug 26, 2024

Description

Add Ethernet support for iMX RT106x, merging the code with existing driver for iMX RT6UL/6ULL in drivers/imx-enet.c

Motivation and Context

https://jira.phoenix-rtos.com/browse/RTOS-507

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

  • Already covered by automatic testing.
  • New test added: (add PR link here).
  • Tested by hand on: armv7a7-imx6ull-evk, armv7m7-imxrt106x-evk, armv7a7-imx6ull-dataro.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

drivers/enet.c Outdated Show resolved Hide resolved
drivers/enet.c Outdated Show resolved Hide resolved
drivers/enet.c Outdated Show resolved Hide resolved
drivers/enet.h Outdated Show resolved Hide resolved
drivers/enet.h Outdated Show resolved Hide resolved
drivers/enet.h Outdated Show resolved Hide resolved
drivers/enet.h Outdated Show resolved Hide resolved
drivers/enet.h Outdated Show resolved Hide resolved
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch from 3185e73 to a582983 Compare August 26, 2024 12:09
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch 3 times, most recently from b8059e4 to 2af33ab Compare September 3, 2024 12:58
drivers/imx-enet.h Outdated Show resolved Hide resolved
drivers/imx-enet.h Outdated Show resolved Hide resolved
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch from 2af33ab to f5e973e Compare September 3, 2024 13:01
drivers/imx-enet.h Outdated Show resolved Hide resolved
@phoenix-rtos phoenix-rtos deleted a comment from github-actions bot Sep 3, 2024
Copy link
Member

@nalajcie nalajcie left a comment

Choose a reason for hiding this comment

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

Partial review as current commit structure prevents efficient code review (the browser crashed with the amount of changes/comments in single commit :)).

Short summary:

  • fix commit message titles (see git history for examples), at least prefix them with module name (also include in commit message bodies what / why was changed), add missing JIRA ID
  • clearly split the changes into separate atomic commits (eg. changing type name is not bringing new functionality and should be a separate change)
  • fix coding style (according to our coding convention / MISRA as described in some comments)
  • please test the code after changes on imx6ull-evk and DTR project (from CI I see that something is broken there), check also if the performance is not degraded

_targets/Makefile.armv7a7-imx6ull Show resolved Hide resolved
drivers/bdring.h Show resolved Hide resolved
drivers/bdring.h Outdated Show resolved Hide resolved
drivers/ephy.c Outdated Show resolved Hide resolved
drivers/pktmem.c Outdated Show resolved Hide resolved
drivers/imx-enet.c Outdated Show resolved Hide resolved
drivers/imx-enet.c Show resolved Hide resolved
drivers/ephy.c Outdated Show resolved Hide resolved
drivers/ephy.c Outdated Show resolved Hide resolved
drivers/ephy.c Outdated Show resolved Hide resolved
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch from f5e973e to 7dbbd7e Compare September 4, 2024 08:03
Copy link

github-actions bot commented Sep 4, 2024

Unit Test Results

7 725 tests  +25   7 008 ✅ +23   39m 0s ⏱️ + 1m 40s
  445 suites + 9     715 💤 ± 0 
    1 files   ± 0       2 ❌ + 2 

For more details on these failures, see this check.

Results for commit bfbf6e1. ± Comparison against base commit 4896655.

This pull request removes 1 and adds 26 tests. Note that renamed tests count towards both.
phoenix-rtos-tests/proc/test_thread_rand ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/proc/test_thread_rand.threads_rand.test_1
phoenix-rtos-tests/mem/mmap ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned
phoenix-rtos-tests/mem/mmap ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned_file
phoenix-rtos-tests/mem/mmap ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/mem/mmap.test_mmap.len__zero
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-qemu:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-qemu:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned_file
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-qemu:phoenix-rtos-tests/mem/mmap.test_mmap.len__zero
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned_file
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mmap.test_mmap.len__zero
phoenix-rtos-tests/mem/mmap ‑ armv7m4-stm32l4x6-nucleo:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned
…

♻️ This comment has been updated with latest results.

drivers/bdring.c Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Show resolved Hide resolved
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch 2 times, most recently from e11d5db to 067dc1b Compare September 6, 2024 15:10
drivers/imx-enet.c Outdated Show resolved Hide resolved
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch 3 times, most recently from 8c41250 to fc32052 Compare September 10, 2024 10:19
@nalajcie nalajcie marked this pull request as draft September 13, 2024 11:23
@nalajcie
Copy link
Member

converting to draft until it will be ready for next review

drivers/pktmem.c Outdated Show resolved Hide resolved
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch 2 times, most recently from 23199d4 to 949cbf1 Compare September 16, 2024 08:29
drivers/pktmem.c Outdated Show resolved Hide resolved
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch 3 times, most recently from b05a49a to ccc4d77 Compare September 17, 2024 11:39
@julianuziemblo julianuziemblo marked this pull request as ready for review September 17, 2024 11:55
Copy link
Member

@nalajcie nalajcie left a comment

Choose a reason for hiding this comment

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

pointed out further issues with the code.

❗ Please confirm that the code was tested on DTR platform (change the PR to draft until then).

Please split into more (smaller, atomic) commits. Every potentially-breaking change needs to be in separate commit - clearly describing WHY it's needed

Please fix commit messages:

  • be more concrete specifying the module in commit message title - here it will mostly be drivers/imx-enet or drivers - don't list all the files changed (that's what git is for)
  • for fixup comment - write in commit WHAT you're fixing, not WHICH commit (I need to dig further to learn what has actually changed), add info about the commit fixed in commit message body
  • add newline after commit message body and before the trailers

drivers/gpio.h Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet.c Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet-regs.h Outdated Show resolved Hide resolved
drivers/imx-enet.c Outdated Show resolved Hide resolved
drivers/pktmem.c Outdated Show resolved Hide resolved
@julianuziemblo julianuziemblo marked this pull request as draft September 23, 2024 12:58
@julianuziemblo julianuziemblo changed the title Add support for ethernet in imxrt1064-evk drivers/imx-enet: Add support for ethernet in imxrt1064-evk Sep 26, 2024
Julian Uziemblo and others added 10 commits September 27, 2024 10:07
change code style to match coding conventions

JIRA: RTOS-507
refactor printfs with repeated format pattern into ephy_printf

JIRA: RTOS-507
first step to add separate implementation files for imx6ull and imxrt106x gpio

JIRA: RTOS-507
remove active value inversion in gpio_wait because the value is already being inverted in gpio_get

JIRA: RTOS-507
fix 7b90e8e:
- add _Atomic specifier to volatile vars used as atomics
- change operations on the vars to atomic_*

JIRA: RTOS-507
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch from ccc4d77 to b4be4d5 Compare October 3, 2024 15:28
drivers/pktmem.c Outdated Show resolved Hide resolved
drivers/pktmem.c Outdated Show resolved Hide resolved
fix packet buffer handling for targets with page size less than packet buffer size

JIRA: RTOS-507
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/enet-imxrt1064-evk branch from b4be4d5 to bfbf6e1 Compare October 3, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants