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]
Message-ID: <CAJ+vNU27TUQC4Dt=RHKDkOZFzCV2kWEoxOy-RYyv0+O=fLE+LQ@mail.gmail.com>
Date:   Thu, 18 Mar 2021 12:58:08 -0700
From:   Tim Harvey <tharvey@...eworks.com>
To:     Jacky Bai <ping.bai@....com>
Cc:     Frieder Schrempf <frieder.schrempf@...tron.de>,
        Abel Vesa <abel.vesa@....com>,
        Dong Aisheng <dongas86@...il.com>,
        Aisheng Dong <aisheng.dong@....com>,
        Rob Herring <robh@...nel.org>, Peng Fan <peng.fan@....com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Anson Huang <anson.huang@....com>,
        devicetree <devicetree@...r.kernel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Adam Ford <aford173@...il.com>,
        Mike Turquette <mturquette@...libre.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Marek Vasut <marek.vasut@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <fabio.estevam@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        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

On Thu, Feb 25, 2021 at 12:28 AM Jacky Bai <ping.bai@....com> wrote:
>
>
>
> > -----Original Message-----
> > From: Frieder Schrempf [mailto:frieder.schrempf@...tron.de]
> > Sent: Thursday, February 25, 2021 4:23 PM
> > To: Abel Vesa <abel.vesa@....com>; Dong Aisheng <dongas86@...il.com>
> > Cc: Aisheng Dong <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>;
> > dl-linux-imx <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.
> >
>
> Before continuing this work, we need to find out a way to resolve the cycling dependency issue between power domain and blk-ctrl.
> it is indeed introduced some troubles in NXP latest internal release when the blk-ctrl driver is added.
>

Jacky,

Any update on this? This is still blocking several drivers and major
functionality of the i.MX8 SoC's in mainline and I would hope this
would be a top priority for NXP.

Best regards,

Tim