| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210625174748.ostcveo6bey4e7a7@pengutronix.de>
Date: Fri, 25 Jun 2021 19:47:48 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Jitao Shi <jitao.shi@...iatek.com>
Cc: Thierry Reding <thierry.reding@...il.com>,
Lee Jones <lee.jones@...aro.org>,
Matthias Brugger <matthias.bgg@...il.com>,
linux-pwm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
srv_heupstream@...iatek.com, yingjoe.chen@...iatek.com,
eddie.huang@...iatek.com, cawa.cheng@...iatek.com,
bibby.hsieh@...iatek.com, ck.hu@...iatek.com, stonea168@....com,
huijuan.xie@...iatek.com
Subject: Re: [PATCH v5 1/3] pwm: mtk-disp: adjust the clocks to avoid them
mismatch
On Wed, Jun 16, 2021 at 04:52:22PM +0800, Jitao Shi wrote:
> The clks "main" and "mm" are prepared in .probe() (and
> unprepared in .remove()). This results in the clocks being on
> during suspend which results in unnecessarily increased power
> consumption.
>
> Remove the clock operations from .probe() and .remove().
> Add the clk_prepare_enable() in .config().
> Add the clk_disable_unprepare() in .disable().
> Remove the MT2701 double buffer comment.
>
> Signed-off-by: Jitao Shi <jitao.shi@...iatek.com>
> ---
> drivers/pwm/pwm-mtk-disp.c | 116 ++++++++++++++++++-------------------
> 1 file changed, 57 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index 9b3ba401a3db..204d76beeb26 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -47,6 +47,7 @@ struct mtk_disp_pwm {
> struct clk *clk_main;
> struct clk *clk_mm;
> void __iomem *base;
> + bool enabled;
> };
>
> static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
> @@ -74,6 +75,22 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> u64 div, rate;
> int err;
>
> + if (!mdp->enabled) {
> + err = clk_prepare_enable(mdp->clk_main);
> + if (err < 0) {
> + dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n",
> + ERR_PTR(err));
> + return err;
> + }
> + err = clk_prepare_enable(mdp->clk_mm);
> + if (err < 0) {
> + dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n",
> + ERR_PTR(err));
> + clk_disable_unprepare(mdp->clk_main);
> + return err;
> + }
> + }
> +
The logic becomes a tad simpler if you prepare_enable unconditionally on
entry and disable_unprepare on exit. Given that in the mdp->enabled ==
true the clocks are already on they are a noop in this case and the
matching disable just decreases the usage count.
> /*
> * Find period, high_width and clk_div to suit duty_ns and period_ns.
> * Calculate proper div value to keep period value in the bound.
> @@ -87,9 +104,13 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> rate = clk_get_rate(mdp->clk_main);
> clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> PWM_PERIOD_BIT_WIDTH;
> - if (clk_div > PWM_CLKDIV_MAX)
> + if (clk_div > PWM_CLKDIV_MAX) {
> + if (!mdp->enabled) {
> + clk_disable_unprepare(mdp->clk_mm);
> + clk_disable_unprepare(mdp->clk_main);
> + }
> return -EINVAL;
> -
> + }
> div = NSEC_PER_SEC * (clk_div + 1);
> period = div64_u64(rate * period_ns, div);
> if (period > 0)
> @@ -98,16 +119,6 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> high_width = div64_u64(rate * duty_ns, div);
> value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
>
> - err = clk_enable(mdp->clk_main);
> - if (err < 0)
> - return err;
> -
> - err = clk_enable(mdp->clk_mm);
> - if (err < 0) {
> - clk_disable(mdp->clk_main);
> - return err;
> - }
> -
> mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> PWM_CLKDIV_MASK,
> clk_div << PWM_CLKDIV_SHIFT);
> @@ -122,10 +133,19 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> mtk_disp_pwm_update_bits(mdp, mdp->data->commit,
> mdp->data->commit_mask,
> 0x0);
> + } else {
> + mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug,
> + mdp->data->bls_debug_mask,
> + mdp->data->bls_debug_mask);
> + mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> + mdp->data->con0_sel,
> + mdp->data->con0_sel);
This is unrelated for this patch, isn't it?
> }
>
> - clk_disable(mdp->clk_mm);
> - clk_disable(mdp->clk_main);
> + if (!mdp->enabled) {
> + clk_disable_unprepare(mdp->clk_mm);
> + clk_disable_unprepare(mdp->clk_main);
> + }
>
> return 0;
> }
> @@ -135,18 +155,25 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> int err;
>
> - err = clk_enable(mdp->clk_main);
> - if (err < 0)
> - return err;
> -
> - err = clk_enable(mdp->clk_mm);
> - if (err < 0) {
> - clk_disable(mdp->clk_main);
> - return err;
> + if (!mdp->enabled) {
> + err = clk_prepare_enable(mdp->clk_main);
> + if (err < 0) {
> + dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n",
> + ERR_PTR(err));
> + return err;
> + }
> + err = clk_prepare_enable(mdp->clk_mm);
> + if (err < 0) {
> + dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n",
> + ERR_PTR(err));
> + clk_disable_unprepare(mdp->clk_main);
> + return err;
> + }
> }
>
> mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
> mdp->data->enable_mask);
> + mdp->enabled = true;
>
> return 0;
> }
> @@ -158,8 +185,11 @@ static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
> 0x0);
>
> - clk_disable(mdp->clk_mm);
> - clk_disable(mdp->clk_main);
> + if (mdp->enabled) {
> + clk_disable_unprepare(mdp->clk_mm);
> + clk_disable_unprepare(mdp->clk_main);
> + }
> + mdp->enabled = false;
> }
>
> static const struct pwm_ops mtk_disp_pwm_ops = {
> @@ -192,58 +222,26 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev)
> if (IS_ERR(mdp->clk_mm))
> return PTR_ERR(mdp->clk_mm);
>
> - ret = clk_prepare(mdp->clk_main);
> - if (ret < 0)
> - return ret;
> -
> - ret = clk_prepare(mdp->clk_mm);
> - if (ret < 0)
> - goto disable_clk_main;
> -
> mdp->chip.dev = &pdev->dev;
> mdp->chip.ops = &mtk_disp_pwm_ops;
> mdp->chip.npwm = 1;
>
> ret = pwmchip_add(&mdp->chip);
> if (ret < 0) {
> - dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> - goto disable_clk_mm;
> + dev_err(&pdev->dev, "pwmchip_add() failed: %pe\n", ERR_PTR(ret));
> + return ret;
> }
>
> platform_set_drvdata(pdev, mdp);
>
> - /*
> - * For MT2701, disable double buffer before writing register
> - * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH.
> - */
> - if (!mdp->data->has_commit) {
> - mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug,
> - mdp->data->bls_debug_mask,
> - mdp->data->bls_debug_mask);
> - mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> - mdp->data->con0_sel,
> - mdp->data->con0_sel);
> - }
> -
Similar than above this is unrelated at least to what you described in
the commit log. Maybe this part is better split into a separate patch?
> return 0;
> -
> -disable_clk_mm:
> - clk_unprepare(mdp->clk_mm);
> -disable_clk_main:
> - clk_unprepare(mdp->clk_main);
> - return ret;
> }
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists