linux-kernel - Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver

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, 25 Feb 2021 09:23:25 +0100
From:   Frieder Schrempf <frieder.schrempf@...tron.de>
To:     Abel Vesa <abel.vesa@....com>, Dong Aisheng <dongas86@...il.com>
Cc:     Dong Aisheng <aisheng.dong@....com>, Rob Herring <robh@...nel.org>,
        Peng Fan <peng.fan@....com>, Jacky Bai <ping.bai@....com>,
        Anson Huang <anson.huang@....com>,
        devicetree <devicetree@...r.kernel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Mike Turquette <mturquette@...libre.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Marek Vasut <marek.vasut@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <fabio.estevam@....com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Adam Ford <aford173@...il.com>,
        linux-clk <linux-clk@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        Lucas Stach <l.stach@...gutronix.de>
Subject: Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver

Hi Abel,

On 17.11.20 15:48, Abel Vesa wrote:
> On 20-11-11 17:13:25, Dong Aisheng wrote:
>> On Tue, Nov 3, 2020 at 7:22 PM Abel Vesa <abel.vesa@....com> wrote:
>> ...
>>> +static int imx_blk_ctl_reset_set(struct reset_controller_dev *rcdev,
>>> +                                 unsigned long id, bool assert)
>>> +{
>>> +       struct imx_blk_ctl_drvdata *drvdata = container_of(rcdev,
>>> +                       struct imx_blk_ctl_drvdata, rcdev);
>>> +       unsigned int offset = drvdata->rst_hws[id].offset;
>>> +       unsigned int shift = drvdata->rst_hws[id].shift;
>>> +       unsigned int mask = drvdata->rst_hws[id].mask;
>>> +       void __iomem *reg_addr = drvdata->base + offset;
>>> +       unsigned long flags;
>>> +       u32 reg;
>>> +
>>> +       if (!assert && !test_bit(1, &drvdata->rst_hws[id].asserted))
>>> +               return -ENODEV;
>>
>> What if consumers call deassert first in probe which seems common in kernel?
>> It seems will fail.
>> e.g.
>> probe() {
>>      reset_control_get()
>>      reset_control_deassert()
>> }
>>
>> Regards
>> Aisheng
>>
> 
> OK, I'm trying to explain here how I know the resets are supposed to be working
> and how the BLK_CTL IP is working.
> 
> 
> First of, the BLK_CTL bits (resets and clocks) all have the HW init (default) values
> as 0. Basically, after the blk_ctl PD is powered on, the resets are deasserted and
> clocks are gated by default. Since the blk_ctl is not the parent of any of the
> consumers in devicetree (the reg maps are entirely different anyway), there is no
> way of ordering the runtime callbacks between the consumer and the blk_ctl. So we
> might end up having the runtime resume callback after the one from EARC (consumer),
> for example, which will basically overwrite the value written by EARC driver with
> whatever was saved on suspend.
> 
> Now, about the usage of the reset bits. AFAICT, it would make more sense to assert
> the reset, then enable the clock, then deassert. This way, you're keeping the
> EARC (consumer) in reset (with the clocks on) until you eventually release it out of
> reset by deasserting. This is how the runtime resume should deal with the reset
> and the clock. As for the runtime suspend, the reset can be entirely ignored as long
> as you're disabling the clock.
> 
> This last part will allow the blk_ctl to make the following assumption:
> if all the clocks are disabled and none of the reset bits are asserted, I can power off.
> 
> Now, I know there are drivers outthere that do assert on suspend, but as long as the
> clocks are disabled, the assert will have no impact. But maybe in their case the reset
> controller cannot power down itself.
> 
> As for the safekeeping of the register, I'll just drop it due to the following arguments:
> 1. all the clocks are gated by default
> 2. all resets are deasserted by default
> 3. when blk_ctl goes down, all the consumers go down. (all have the same PD)
> 
>  From 1 and 2 results the IP will not be running and from 3 results the HW state
> of every IP becomes HW init state.

Are there any plans to continue this work? As BLK-CTL it is not only 
relevant for the i.MX8MP, but also for i.MX8MM and i.MX8MN, it would be 
nice to get this ready in order to prepare for proper graphics/display 
support.

Thanks
Frieder