From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> To: lgirdwood@gmail.com, broonie@kernel.org, "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, linux-pwm@vger.kernel.org, linux-amlogic@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Heiner Kallweit <hkallweit1@gmail.com>, Dmitry Rokosov <ddrokosov@sberdevices.ru>, Martin Blumenstingl <martin.blumenstingl@googlemail.com> Subject: [RFC PATCH v2 3/3] regulator: pwm-regulator: Manage boot-on with disabled PWM channels Date: Sat, 13 Jan 2024 23:46:28 +0100 [thread overview] Message-ID: <20240113224628.377993-4-martin.blumenstingl@googlemail.com> (raw) In-Reply-To: <20240113224628.377993-1-martin.blumenstingl@googlemail.com> Odroid-C1 uses a Monolithic Power Systems MP2161 controlled via PWM for the VDDEE voltage supply of the Meson8b SoC. Commit 6b9352f3f8a1 ("pwm: meson: modify and simplify calculation in meson_pwm_get_state") results in my Odroid-C1 crashing with memory corruption in many different places (seemingly at random). It turns out that this is due to a currently not supported corner case. The VDDEE regulator can generate between 860mV (duty cycle of ~91%) and 1140mV (duty cycle of 0%). We consider it to be enabled by the bootloader (which is why it has the regulator-boot-on flag in .dts) as well as being always-on (which is why it has the regulator-always-on flag in .dts) because the VDDEE voltage is generally required for the Meson8b SoC to work. The public S805 datasheet [0] states on page 17 (where "A5" refers to the Cortex-A5 CPU cores): [...] So if EE domains is shut off, A5 memory is also shut off. That does not matter. Before EE power domain is shut off, A5 should be shut off at first. It turns out that at least some bootloader versions are keeping the PWM output disabled. This is not a problem due to the specific design of the regulator: when the PWM output is disabled the output pin is pulled LOW, effectively achieving a 0% duty cycle (which in return means that VDDEE voltage is at 1140mV). The problem comes when the pwm-regulator driver tries to initialize the PWM output. To do so it reads the current state from the hardware, which is: period: 3666ns duty cycle: 3333ns (= ~91%) enabled: false Then those values are translated using the continuous voltage range to 860mV. Later, when the regulator is being enabled (either by the regulator core due to the always-on flag or first consumer - in this case the lima driver for the Mali-450 GPU) the pwm-regulator driver tries to keep the voltage (at 860mV) and just enable the PWM output. This is when things start to go wrong as the typical voltage used for VDDEE is 1100mV. Commit 6b9352f3f8a1 ("pwm: meson: modify and simplify calculation in meson_pwm_get_state") triggers above condition as before that change period and duty cycle were both at 0. Since the change to the pwm-meson driver is considered correct the solution is to be found in the pwm-regulator driver. Update the duty cycle during driver probe if the regulator is flagged as boot-on so that a call to pwm_regulator_enable() (by the regulator core during initialization of a regulator flagged with boot-on) without any preceding call to pwm_regulator_set_voltage() does not change the output voltage. [0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- Changes from v1 -> v2: - This patch is new and the idea is to keep the voltage regulator description in device-tree as-is (which means: support for 860mV to 1140mV). Instead we allow the pwm-regulator driver to preserve the output voltage at boot when enabling the regulator. - note: Mark Brown said in v1 "I'd expect a change in the init_state() function". That function is not updated as it's only relevant for regulators with a voltage table - on Odroid-C1 we have a continuous regulator though. drivers/regulator/pwm-regulator.c | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c index d27b9a7a30c9..60cfcd741c2a 100644 --- a/drivers/regulator/pwm-regulator.c +++ b/drivers/regulator/pwm-regulator.c @@ -323,6 +323,32 @@ static int pwm_regulator_init_continuous(struct platform_device *pdev, return 0; } +static int pwm_regulator_init_boot_on(struct platform_device *pdev, + struct pwm_regulator_data *drvdata, + const struct regulator_init_data *init_data) +{ + struct pwm_state pstate; + + if (!init_data->constraints.boot_on || drvdata->enb_gpio) + return 0; + + pwm_get_state(drvdata->pwm, &pstate); + if (pstate.enabled) + return 0; + + /* + * Update the duty cycle so the output does not change + * when the regulator core enables the regulator (and + * thus the PWM channel). + */ + if (pstate.polarity == PWM_POLARITY_INVERSED) + pstate.duty_cycle = pstate.period; + else + pstate.duty_cycle = 0; + + return pwm_apply_might_sleep(drvdata->pwm, &pstate); +} + static int pwm_regulator_probe(struct platform_device *pdev) { const struct regulator_init_data *init_data; @@ -382,6 +408,13 @@ static int pwm_regulator_probe(struct platform_device *pdev) if (ret) return ret; + ret = pwm_regulator_init_boot_on(pdev, drvdata, init_data); + if (ret) { + dev_err(&pdev->dev, "Failed to apply boot_on settings: %d\n", + ret); + return ret; + } + regulator = devm_regulator_register(&pdev->dev, &drvdata->desc, &config); if (IS_ERR(regulator)) { -- 2.43.0
WARNING: multiple messages have this Message-ID (diff)
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> To: lgirdwood@gmail.com, broonie@kernel.org, "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, linux-pwm@vger.kernel.org, linux-amlogic@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Heiner Kallweit <hkallweit1@gmail.com>, Dmitry Rokosov <ddrokosov@sberdevices.ru>, Martin Blumenstingl <martin.blumenstingl@googlemail.com> Subject: [RFC PATCH v2 3/3] regulator: pwm-regulator: Manage boot-on with disabled PWM channels Date: Sat, 13 Jan 2024 23:46:28 +0100 [thread overview] Message-ID: <20240113224628.377993-4-martin.blumenstingl@googlemail.com> (raw) In-Reply-To: <20240113224628.377993-1-martin.blumenstingl@googlemail.com> Odroid-C1 uses a Monolithic Power Systems MP2161 controlled via PWM for the VDDEE voltage supply of the Meson8b SoC. Commit 6b9352f3f8a1 ("pwm: meson: modify and simplify calculation in meson_pwm_get_state") results in my Odroid-C1 crashing with memory corruption in many different places (seemingly at random). It turns out that this is due to a currently not supported corner case. The VDDEE regulator can generate between 860mV (duty cycle of ~91%) and 1140mV (duty cycle of 0%). We consider it to be enabled by the bootloader (which is why it has the regulator-boot-on flag in .dts) as well as being always-on (which is why it has the regulator-always-on flag in .dts) because the VDDEE voltage is generally required for the Meson8b SoC to work. The public S805 datasheet [0] states on page 17 (where "A5" refers to the Cortex-A5 CPU cores): [...] So if EE domains is shut off, A5 memory is also shut off. That does not matter. Before EE power domain is shut off, A5 should be shut off at first. It turns out that at least some bootloader versions are keeping the PWM output disabled. This is not a problem due to the specific design of the regulator: when the PWM output is disabled the output pin is pulled LOW, effectively achieving a 0% duty cycle (which in return means that VDDEE voltage is at 1140mV). The problem comes when the pwm-regulator driver tries to initialize the PWM output. To do so it reads the current state from the hardware, which is: period: 3666ns duty cycle: 3333ns (= ~91%) enabled: false Then those values are translated using the continuous voltage range to 860mV. Later, when the regulator is being enabled (either by the regulator core due to the always-on flag or first consumer - in this case the lima driver for the Mali-450 GPU) the pwm-regulator driver tries to keep the voltage (at 860mV) and just enable the PWM output. This is when things start to go wrong as the typical voltage used for VDDEE is 1100mV. Commit 6b9352f3f8a1 ("pwm: meson: modify and simplify calculation in meson_pwm_get_state") triggers above condition as before that change period and duty cycle were both at 0. Since the change to the pwm-meson driver is considered correct the solution is to be found in the pwm-regulator driver. Update the duty cycle during driver probe if the regulator is flagged as boot-on so that a call to pwm_regulator_enable() (by the regulator core during initialization of a regulator flagged with boot-on) without any preceding call to pwm_regulator_set_voltage() does not change the output voltage. [0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- Changes from v1 -> v2: - This patch is new and the idea is to keep the voltage regulator description in device-tree as-is (which means: support for 860mV to 1140mV). Instead we allow the pwm-regulator driver to preserve the output voltage at boot when enabling the regulator. - note: Mark Brown said in v1 "I'd expect a change in the init_state() function". That function is not updated as it's only relevant for regulators with a voltage table - on Odroid-C1 we have a continuous regulator though. drivers/regulator/pwm-regulator.c | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c index d27b9a7a30c9..60cfcd741c2a 100644 --- a/drivers/regulator/pwm-regulator.c +++ b/drivers/regulator/pwm-regulator.c @@ -323,6 +323,32 @@ static int pwm_regulator_init_continuous(struct platform_device *pdev, return 0; } +static int pwm_regulator_init_boot_on(struct platform_device *pdev, + struct pwm_regulator_data *drvdata, + const struct regulator_init_data *init_data) +{ + struct pwm_state pstate; + + if (!init_data->constraints.boot_on || drvdata->enb_gpio) + return 0; + + pwm_get_state(drvdata->pwm, &pstate); + if (pstate.enabled) + return 0; + + /* + * Update the duty cycle so the output does not change + * when the regulator core enables the regulator (and + * thus the PWM channel). + */ + if (pstate.polarity == PWM_POLARITY_INVERSED) + pstate.duty_cycle = pstate.period; + else + pstate.duty_cycle = 0; + + return pwm_apply_might_sleep(drvdata->pwm, &pstate); +} + static int pwm_regulator_probe(struct platform_device *pdev) { const struct regulator_init_data *init_data; @@ -382,6 +408,13 @@ static int pwm_regulator_probe(struct platform_device *pdev) if (ret) return ret; + ret = pwm_regulator_init_boot_on(pdev, drvdata, init_data); + if (ret) { + dev_err(&pdev->dev, "Failed to apply boot_on settings: %d\n", + ret); + return ret; + } + regulator = devm_regulator_register(&pdev->dev, &drvdata->desc, &config); if (IS_ERR(regulator)) { -- 2.43.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic
next prev parent reply other threads:[~2024-01-13 22:47 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-01-13 22:46 [RFC PATCH v2 0/3] regulator: pwm-regulator: Fixes for disabled PWMs at boot Martin Blumenstingl 2024-01-13 22:46 ` Martin Blumenstingl 2024-01-13 22:46 ` [RFC PATCH v2 1/3] regulator: pwm-regulator: Add validity checks in continuous .get_voltage Martin Blumenstingl 2024-01-13 22:46 ` Martin Blumenstingl 2024-01-13 22:46 ` [RFC PATCH v2 2/3] regulator: pwm-regulator: Calculate the output voltage for disabled PWMs Martin Blumenstingl 2024-01-13 22:46 ` Martin Blumenstingl 2024-01-13 22:46 ` Martin Blumenstingl [this message] 2024-01-13 22:46 ` [RFC PATCH v2 3/3] regulator: pwm-regulator: Manage boot-on with disabled PWM channels Martin Blumenstingl 2024-01-22 20:44 ` [RFC PATCH v2 0/3] regulator: pwm-regulator: Fixes for disabled PWMs at boot Mark Brown 2024-01-22 20:44 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20240113224628.377993-4-martin.blumenstingl@googlemail.com \ --to=martin.blumenstingl@googlemail.com \ --cc=broonie@kernel.org \ --cc=ddrokosov@sberdevices.ru \ --cc=hkallweit1@gmail.com \ --cc=lgirdwood@gmail.com \ --cc=linux-amlogic@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=u.kleine-koenig@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.