zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.59k stars 6.48k forks source link

regulators: adp5360 driver is setting wrong bits for BUCK_ILIM #71711

Closed AaronFontaine-DojoFive closed 2 months ago

AaronFontaine-DojoFive commented 5 months ago

Describe the bug Zephyr contains a driver for the ADP5360 PMIC. This driver is only for the regulators and is capable of controlling and configuring the two separate regulators inside the ADP5360 (buck and buck-boost). The DTS bindings YAML for the ADP5360 (dts/bindings/regulator/adi,adp5360-regulator.yaml) defines an adi,ilim-milliamp property for the regulator nodes in the devicetree. This setting is applied in the regulator_adp5360_init() function in drivers/regulator/regulator_adp5360.c during Zephyr initialization. This function is called twice, once for the buck regulator and once for the buck-boost regulator.

The ADP5360 datasheet shows that the ILIM field for the buck regulator is bits 5-4 of register 0x29 (Buck Configure) and that the ILIM field for the buck boost regulator is bits 5-3 of register 0x2B (Buck Boost Configure).

The bug in regulator_adp5360.c is that the mask and position defines for the buck ILIM field are wrong (ADP5360_BUCK_CFG_BUCK_ILIM_MSK and ADP5360_BUCK_CFG_BUCK_ILIM_POS). They are defining it as bits 5-3 and when it should be 5-4 only. This causes the adi,ilim-milliamp property to be applied incorrectly for the buck regulator, causing an incorrect value in BUCK_ILIM and overwriting the BUCK_MODE setting.

This bug was introduced in commit e05df8faf1e4c474014261d849ba41830416b61b and merged in pull request 58184,

To Reproduce Add an adi,adp5360 compatible node to your devicetree and set the adi,ilim-milliamp property of the BUCK regulator child node. Example:

&i2c3 {
    compatible = "nordic,nrf-twim";
    status = "okay";
    pinctrl-0 = <&i2c3_default>;
    pinctrl-1 = <&i2c3_sleep>;
    pinctrl-names = "default", "sleep";
    pmic@46 {
        compatible = "adi,adp5360";
        reg = <0x46>;
        regulators {
            compatible = "adi,adp5360-regulator";
            pmic_1v8: BUCK {
                regulator-always-on;
                adi,ilim-milliamp = <200>;
            };
            pmic_5v: BUCKBOOST {
                adi,ilim-milliamp = <200>;
            };
        };
    };
};

In this example, 200mA will result in an ilim_idx value of 1. When shifted to the incorrect position of 3U instead of 4U, it sets the BUCK_MODE bit (changing the regulator from Hysteresis to FPWM mode) and clears BUCK_ILIM (setting the limit to 100mA instead of 200). I verified this bug by stepping through regulator_adp5360_init() in the debugger. The adp5360 driver does not implement the functions to read the current limit, but there is another way to verify this bug without the debugger.

Get a reference to the buck regulator node and print out its mode setting.

const struct device *const m_adp5360_buck_dev = DEVICE_DT_GET(DT_NODELABEL(pmic_1v8));

void printRegualtorSetting(void)
{
    regulator_mode_t buck_reg_mode;
    int rc;

    rc = regulator_get_mode(m_adp5360_buck_dev, &buck_reg_mode);
    if (rc != 0) {
        LOG_ERR("Failed to get PMIC data");
        return;
    }

    switch (buck_reg_mode) {
    case ADP5360_MODE_HYS:
        LOG_INF("PMIC buck mode = Hysteresis");
        break;
    case ADP5360_MODE_PWM:
        LOG_INF("PMIC buck mode = PWM");
        break;
    default:
        LOG_INF("PMIC buck mode = Unknown");
        break;
    }
}

Without the adi,ilim-milliamp = <200>; setting in the devicetree, this will print "PMIC buck mode = Hysteresis". With the adi,ilim-milliamp = <200>;, it will print "PMIC buck mode = PWM".

Expected behavior The ILIM setting in the devicetree correctly sets the ILIM field in the device register and does not adversely impact the regulator mode.

Impact Minimal. We probably do not need to set the current limit. We can around the issue if necessary.

Logs and console output Without the adi,ilim-milliamp = <200>; setting

00> [00070424] <inf> main: PMIC buck voltage = 1.800 V
00> [00070424] <inf> main: PMIC buck mode = Hysteresis
00> [00070425] <inf> main: PMIC buckboost voltage = 3.300 V

With the adi,ilim-milliamp = <200>; setting

00> [00070450] <inf> main: PMIC buck voltage = 1.800 V
00> [00070450] <inf> main: PMIC buck mode = PWM
00> [00070451] <inf> main: PMIC buckboost voltage = 3.300 V

Environment (please complete the following information):

github-actions[bot] commented 5 months ago

Hi @AaronFontaine-DojoFive! We appreciate you submitting your first issue for our open-source project. šŸŒŸ

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. šŸ¤–šŸ’™

aasinclair commented 5 months ago

Hi @AaronFontaine-DojoFive. Thanks for your thorough investigations into this. Are you interested in creating a PR to fix this issue?

github-actions[bot] commented 3 months ago

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.