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

rtl872x: overshoot issue #2680

Merged
merged 4 commits into from Aug 10, 2023
Merged

rtl872x: overshoot issue #2680

merged 4 commits into from Aug 10, 2023

Conversation

YutingYou
Copy link
Contributor

Problem

During the EVT test, we observed significant overshoot. This issue occurs on every high-speed bus, including SPI, I2C, PWM, I2S, and DMIC.

Solution

There are four drive strength can be configured for RTL872x, all the GPIO were configured to PAD_DRV_STRENGTH_3 at boot.

  • PAD_DRV_STRENGTH_0: 4mA
  • PAD_DRV_STRENGTH_1: 8mA (Severe overshoot, not recommended to use it)
  • PAD_DRV_STRENGTH_2: 12mA
  • PAD_DRV_STRENGTH_3: 16mA (Severe overshoot, not recommended to use it)

We compared the overshoot issue with each drive strength configuration and found PAD_DRV_STRENGTH_0 and PAD_DRV_STRENGTH_2 were better than PAD_DRV_STRENGTH_1 and PAD_DRV_STRENGTH_3

Below is the clock pin of SPI0@50MHz
Screenshot 2023-08-02 at 15 19 33

Selecting PAD_DRV_STRENGTH_2 could be a solution to avoid the overshoot issue while maintaining a high drive strength.

Below, you will find a more detailed comparison between PAD_DRV_STRENGTH_2 and PAD_DRV_STRENGTH_3 across different buses on the Orson platform.

Screenshot 2023-08-04 at 16 01 42

Steps to Test

  • Compare the waveforms of each high-speed bus on P2 or Orson before and after applying the fix.

References

[CH119867]
[CH119866]
[CH120087]
[CH119868]


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@@ -73,6 +73,8 @@ PinMode hal_gpio_get_mode(hal_pin_t pin);
void hal_gpio_write(hal_pin_t pin, uint8_t value);
int32_t hal_gpio_read(hal_pin_t pin);
uint32_t hal_gpio_pulse_in(hal_pin_t pin, uint16_t value);
hal_gpio_drive_t hal_gpio_get_drive_strength(hal_pin_t pin);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: If we are later to expose this in dynalib might perhaps be better to do int hal_gpio_get_drive_strength(hal_pin_t hal_gpio_drive_t* drive) to catch errors (e.g. wrong pin, input only pin etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

return drive;
}

int hal_gpio_configure_drive_strength(hal_pin_t pin, hal_gpio_drive_t drive) {
Copy link
Member

Choose a reason for hiding this comment

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

hal_gpio_config_t::drive_strength should also be handled in hal_gpio_configure, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, set to HAL_GPIO_DRIVE_DEFAULT in the hal_gpio_configure

Copy link
Member

Choose a reason for hiding this comment

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

Hm. What I meant is that it should be called in hal_gpio_configure with that field as argument, so hal_gpio_configure(pin, config->drive_strength)

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 misunderstood that, I'll rework this API

@scott-brust scott-brust added this to the 5.5.0-rc.1 milestone Aug 7, 2023
hal/src/rtl872x/gpio_hal.cpp Show resolved Hide resolved
return SYSTEM_ERROR_NONE;
}

int hal_gpio_configure_drive_strength(hal_pin_t pin, hal_gpio_drive_t drive) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be clearer if this function was renamed hal_gpio_set_drive_strength() to match hal_gpio_get_drive_strength()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'd also prefer hal_gpio_set_drive_strength(), I intended to keep it consistent with hal_gpio_configure, well, let's rename it.

hal/src/rtl872x/gpio_hal.cpp Outdated Show resolved Hide resolved
@YutingYou YutingYou merged commit a4ae355 into develop Aug 10, 2023
12 checks passed
@YutingYou YutingYou deleted the fix/rtl872x-overshoot-issue branch August 10, 2023 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants