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

[efm32_probe] Fix detection of DI v2 #2069

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

mmmspatz
Copy link
Contributor

@mmmspatz mmmspatz commented Feb 3, 2025

Detailed description

#402 introduced a bug with silabs devices (efm32/efr32) related to the parsing of the DI (Device Info) page. An example is efr32xg13, which I believe would currently be broken. I do not have any of the relevant devices and cannot test this, but I noticed it while playing with adding support for the newer "series 2" parts (eg. EFRMG24).

The driver currently knows about two versions (it calls them "version 1" and "version 2") of the DI page with completely different layouts. It decides which version to use by:

  1. Reading the EUI64 from the location specified by DI layout version 1
  2. checking if the 24 bit OUI therein is equal to EFM32_V1_DI_EUI_SILABS, in which case layout version 1 is used
  3. If not, checking if that same OUI is instead equal to EFM32_V2_DI_EUI_ENERGYMICRO, in which case version 2 is used
  4. Defaulting to version 1 if neither OUI is matched

The problem with this if EFM32_V2_DI_EUI_ENERGYMICRO were to appear in the DI page, it woud be in the location specified by DI layout version 2, and so we need to perform another read from there. There's no way for the current code to ever detect v2.

So, that is the change this PR makes: Also try reading the OUI per layout version 2 (from the EUI48, because there is no EUI64 in v2). This is the behavior originally introduced in #372.

As #402 discusses, this strategy of relying on the OUI to differentiate parts versions is bad in the first place because there are over 100 OUIs assigned to Silicon Labs, and it's not actually known which devices use which OUI. A better idea might be to use the PARTNO value from the IDCODE register. It is mentioned in AN1303 that all series 2 parts (so, not what this driver supports) have IDCODE == 0x6BA02477, perhaps there is similar simple rule for the older parts supported by this driver?

Your checklist for this pull request

@mmmspatz mmmspatz marked this pull request as ready for review February 3, 2025 02:07
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

We can see just one issue remaining which is to do with types/const, but otherwise this looks great. Please note your commit message needs to conform with the contribution guidelines for the repo. efm32: as the prefix not that square bracket expression.

With that taken care of, we'll be happy to approve this and merge as a fix for v2.0 as this looks like a regression as you noted.

@dragonmux dragonmux added Bug Confirmed bug Regression Bug caused by a regression labels Feb 3, 2025
@dragonmux dragonmux added this to the v2.0 release milestone Feb 3, 2025
@dragonmux
Copy link
Member

dragonmux commented Feb 3, 2025

A better idea might be to use the PARTNO value from the IDCODE register. It is mentioned in AN1303 that all series 2 parts (so, not what this driver supports) have IDCODE == 0x6BA02477, perhaps there is similar simple rule for the older parts supported by this driver?

This is an ARM part number, so is not unique or identifying to these devices in any way. This only tells us that we're talking with a ADIv5 DPv2 part. Every ADIv5 DPv2 part shares this ID code (modulo the first nibble (6) which is a version number). 477 -> 43b when decoded, which is ARM's JEP-106. ba02 is the partno for a DP w/ the last nibble (2) indicating version. ba00 -> DPv0, ba01 -> DPv1, ba02 -> DPv2, and ba03 -> DPv3 (ADIv6).

If any unique identifiers are available in that way, it would be from the ROM tables or TARGETID register, which can contain a Vendor part ID.

EFM32_V2_DI_EUI_ENERGYMICRO will never be read by efm32_v1_read_eui64(),
because the EUI itself is in a different location in DI v2. Add back
efm32_v2_read_eui48() and use it.
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

LGTM, merging. Thank you for the contribution!

@dragonmux dragonmux merged commit 2112748 into blackmagic-debug:main Feb 3, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bug Regression Bug caused by a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants