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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 24 Feb 2022 16:21:16 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Michael Turquette <mturquette@...libre.com>,
        Taniya Das <tdas@...eaurora.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Rajendra Nayak <rnayak@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        Taniya Das <tdas@...eaurora.org>
Subject: Re: [v2 1/2] clk: qcom: gdsc: Add support to update GDSC transition delay

Quoting Taniya Das (2022-02-23 10:56:05)
> GDSCs have multiple transition delays which are used for the GDSC FSM
> states. Older targets/designs required these values to be updated from
> gdsc code to certain default values for the FSM state to work as
> expected. But on the newer targets/designs the values updated from the
> GDSC driver can hamper the FSM state to not work as expected.
> 
> On SC7180 we observe black screens because the gdsc is being
> enabled/disabled very rapidly and the GDSC FSM state does not work as
> expected. This is due to the fact that the GDSC reset value is being
> updated from SW.
> 
> Thus add support to update the transition delay from the clock
> controller gdscs as required.
> 
> Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
> Signed-off-by: Taniya Das <tdas@...eaurora.org>

Applied to clk-fixes with Bjorn's reviewed-by. One note below, please
make this improvement.

> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index d7cc4c21a9d4..ad313d7210bd 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  /*
> - * Copyright (c) 2015, 2017-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2015, 2017-2018, 2022, The Linux Foundation. All rights reserved.
>   */
> 
>  #ifndef __QCOM_GDSC_H__
> @@ -22,6 +22,9 @@ struct reset_controller_dev;
>   * @cxcs: offsets of branch registers to toggle mem/periph bits in
>   * @cxc_count: number of @cxcs
>   * @pwrsts: Possible powerdomain power states
> + * @en_rest_wait_val: transition delay value for receiving enr ack signal
> + * @en_few_wait_val: transition delay value for receiving enf ack signal
> + * @clk_dis_wait_val: transition delay value for halting clock
>   * @resets: ids of resets associated with this gdsc
>   * @reset_count: number of @resets
>   * @rcdev: reset controller
> @@ -36,6 +39,9 @@ struct gdsc {
>         unsigned int                    clamp_io_ctrl;
>         unsigned int                    *cxcs;
>         unsigned int                    cxc_count;
> +       unsigned int                    en_rest_wait_val;
> +       unsigned int                    en_few_wait_val;
> +       unsigned int                    clk_dis_wait_val;

Bjorn pointed out the usage of unsigned int is too big. These are 4-bits
wide fields in the hardware.

We should pack these into a u16 and then shift it and write it into
place if it is non-zero. That means the driver author has to know all
the values, but that sounds OK to me given that they're already changing
something from the hardware defaults. This will save space in the
vmlinux for however many gdscs there are declared. We should have a
macro for this too so we can pack all the values together and then just
write it out directly without having to know the shifts and stuff.

#define GDSC_WAIT_VALS(en_rest, en_few, clk_dis)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ