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

[BACKPORT] Ethernet : changes were made to support ethernet #27

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

garfieldG
Copy link

-unicast
-broadcast
-multicast
-no SDIO lines

@dagar
Copy link
Member

dagar commented Aug 2, 2018

Can you look at the build failures?

@garfieldG
Copy link
Author

@dagar No build failures now:)

@dagar
Copy link
Member

dagar commented Aug 6, 2018

Just to be clear, all of this code actually exists in upstream NuttX (master)?

@garfieldG
Copy link
Author

@dagar No, not all of it, most of it

@dagar
Copy link
Member

dagar commented Aug 7, 2018

@dagar No, not all of it, most of it

We need to be clear on that, otherwise the non upstream changes might be lost when we update NuttX.

Does it correspond with a small set of commits that could actually be cherry-picked back?

@garfieldG
Copy link
Author

No, actually there is just one big commit with all changes.
There is the second commit that can be cherry-picked later on, but it's a very small change and just an addition to the big commit.
What do you suggest? Is there something I can do now to change it?

@dagar
Copy link
Member

dagar commented Aug 8, 2018

As discussed please split into 1 commit then is a clean backport from upstream NuttX that we can merge now. Then we can look at your other changes separately and figure out where they belong.

@garfieldG
Copy link
Author

@dagar Just now saw that in Firmware you requested to separate to two different PRs.
In this PR I already separated to two different commits before seeing it (remembered we discussed about two different commits, not two different PRs).
Do you want me to split this one also to two different PRs or is it okay to leave it and keep it as two separated commits and one PR?

@dagar
Copy link
Member

dagar commented Aug 9, 2018

It depends on the actual contents. The portion that's a pure backport from upstream NuttX should be safe to merge immediately. The rest will need a bit more scrutiny and we should consider getting it upstream as well to reduce future maintenance burden. Does that make sense?

@@ -3430,11 +3430,14 @@ static inline void stm32_ethgpioconfig(FAR struct stm32_ethmac_s *priv)
#if defined(CONFIG_STM32_MII) || defined(CONFIG_STM32_RMII)

/* MDC and MDIO are common to both modes */
Copy link
Member

Choose a reason for hiding this comment

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

@garfieldG - Since you are closer to the problem, I need your help to understand this change.

What was the problem you are trying to solve? What it different in your HW and why are this changes needed?

Copy link
Author

Choose a reason for hiding this comment

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

My board has an ethernet switch, not only a PHY device.
Most Phy devices use MDIO lines and I guess that might be the reason why it wasn't referred to until now, but my board communicates with the switch through SPI.

Copy link
Member

Choose a reason for hiding this comment

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

@garfieldG - I am starting. to get the picture. The custom board you have has a PHY that is supported in nuttx but not wired the same.

Does it uses RMMI? Where is the SPI code?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, Ethernet PHY (usually) has 2 communication links to the MAC (the CPU hardware). Usually the 100Mbit/sec data is over MII protocol (either RMII, MII or GMII). but the control (on/off, 100mbit/10mbit, HD/FD and so on) is done by other lines, the default is MDIO standard. It seems like Nuttx handles this standard (you can see it in stm32_ethernet code).
When the PHY is a switch MDIO is less relevant, so you need other communication. In our case its a complicated protocol over SPI lines.
The SPI code is not relevant, because the two protocols are independent. We actually don't do much over the SPI, just monitoring the link (up/down and so on)

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@garfieldG - I am not familiar enough with what you are trying to accomplish. What I was trying and failing at understanding so far, is: Is the change you need to make to a driver in NuttX' stmf7 code base a change specific to just your HW are is it something that would benefit upstream Nuttx? For example, a bug fix or a set of CONFIG_xxx that are wrong in upstream.

We can have a call to discuss this in real time if need be. Skype:david_s5

Copy link
Author

Choose a reason for hiding this comment

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

@davids5
As far as I can see, for now, it is specific to our HW (I I didn't see any reference to a board without MDIO lines), I think it's best we'll leave it for now and in the future if there will be any need for this change I'll be here:)

@garfieldG
Copy link
Author

Makes total sense.
But my question still stands, I don't really know if you can merge only part of the commits in pull request (My work system is a bit different, so never had to deal with merging a pull request) so I don't know if I should separate the commits to different pull requests.

@garfieldG
Copy link
Author

@dagar

@davids5
Copy link
Member

davids5 commented Aug 14, 2018

@garfieldG a PR brings in all the commits so we would need to split it. This is possible with branching your branch and using "git rebase -i" to drop the commits. You could also just cherry pick them to 2 new branches.

@garfieldG
Copy link
Author

@davids5 I'll change it so that only the backport commit will be here and I'll delete the second commit for now

@davids5
Copy link
Member

davids5 commented Aug 15, 2018

@garfieldG Perfect!

@garfieldG
Copy link
Author

@davids5 Hi, is there something new regarding the Ethernet?

@davids5
Copy link
Member

davids5 commented Aug 26, 2018

@garfieldG @dagar was going to run a regression test on HW he has. Once he OK's it, it will be merged.

@dagar dagar merged commit 0ffe7ce into PX4:px4_firmware_nuttx-7.22+ Sep 6, 2018
@garfieldG garfieldG deleted the Ethernet branch October 2, 2018 11:03
PX4BuildBot pushed a commit that referenced this pull request Jan 5, 2020
arch/arm/src/stm32/stm32_fmc.c:  Missing semicolons caused compilation errors.
PX4BuildBot pushed a commit that referenced this pull request Oct 11, 2024
set CONFIG_PRIORITY_INHERITANCE=y
set CONFIG_SEM_PREALLOCHOLDERS=0 or CONFIG_SEM_PREALLOCHOLDERS=8

    #24 0x4dcab71 in __assert assert/lib_assert.c:37
    #25 0x4d6b0e9 in nxsem_destroyholder semaphore/sem_holder.c:602
    #26 0x4d80cf7 in nxsem_destroy semaphore/sem_destroy.c:80
    #27 0x4d80db9 in sem_destroy semaphore/sem_destroy.c:120
    #28 0x4dcb077 in nxmutex_destroy misc/lib_mutex.c:122
    #29 0x4dc6611 in pipecommon_freedev pipes/pipe_common.c:117
    #30 0x4dc7fdc in pipecommon_close pipes/pipe_common.c:397
    #31 0x4ed4f6d in file_close vfs/fs_close.c:78
    #32 0x6a91133 in local_free local/local_conn.c:184
    #33 0x6a92a9c in local_release local/local_release.c:129
    #34 0x6a91d1a in local_subref local/local_conn.c:271
    #35 0x6a75767 in local_close local/local_sockif.c:797
    #36 0x4e978f6 in psock_close socket/net_close.c:102
    #37 0x4eed1b9 in sock_file_close socket/socket.c:115
    #38 0x4ed4f6d in file_close vfs/fs_close.c:78
    #39 0x4ed1459 in nx_close_from_tcb inode/fs_files.c:754
    #40 0x4ed1501 in nx_close inode/fs_files.c:781
    #41 0x4ed154a in close inode/fs_files.c:819
    #42 0x6bcb9ce in property_get kvdb/client.c:307
    #43 0x6bcd465 in property_get_int32 kvdb/common.c:270
    #44 0x5106c9a in tz_offset_restore app/miwear_bluetooth.c:745
    #45 0x510893f in miwear_bluetooth_main app/miwear_bluetooth.c:1033
    #46 0x4dcf5c8 in nxtask_startup sched/task_startup.c:70
    #47 0x4d70873 in nxtask_start task/task_start.c:134
    #48 0x4e04a07 in pre_start sim/sim_initialstate.c:52

Signed-off-by: ligd <liguiding1@xiaomi.com>
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.

3 participants