linux-kernel - Re: [PATCH 11/17] clk: imx: Add blk

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] [day] [month] [year] [list]

Message-ID: <20200812072854.4r4d6cz5cprezlof@fsr-ub1664-175>
Date:   Wed, 12 Aug 2020 10:28:54 +0300
From:   Abel Vesa <abel.vesa@....com>
To:     Philipp Zabel <p.zabel@...gutronix.de>
Cc:     Mike Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <fabio.estevam@....com>,
        Anson Huang <anson.huang@....com>,
        Dong Aisheng <aisheng.dong@....com>,
        Peng Fan <peng.fan@....com>, Fugang Duan <fugang.duan@....com>,
        NXP Linux Team <linux-imx@....com>,
        linux-arm-kernel@...ts.infradead.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-clk@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 11/17] clk: imx: Add blk_ctrl combo driver

On 20-07-30 11:39:22, Philipp Zabel wrote:
> On Thu, 2020-07-30 at 11:55 +0300, Abel Vesa wrote:
> > On 20-07-29 14:46:28, Philipp Zabel wrote:
> > > Hi Abel,
> > > 
> > > On Wed, 2020-07-29 at 15:07 +0300, Abel Vesa wrote:
> > > > On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
> > 
> > [...]
> > 
> > > > +
> > > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> > > > +				  unsigned long id, bool assert)
> > > > +{
> > > > +	struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> > > > +			struct imx_blk_ctrl_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) {
> > > > +		pm_runtime_get_sync(rcdev->dev);
> > > > +		spin_lock_irqsave(&drvdata->lock, flags);
> > > > +		reg = readl(reg_addr);
> > > > +		writel(reg & ~(mask << shift), reg_addr);
> > > > +		spin_unlock_irqrestore(&drvdata->lock, flags);
> > > > +	} else {
> > > > +		spin_lock_irqsave(&drvdata->lock, flags);
> > > > +		reg = readl(reg_addr);
> > > > +		writel(reg | (mask << shift), reg_addr);
> > > > +		spin_unlock_irqrestore(&drvdata->lock, flags);
> > > > +		pm_runtime_put(rcdev->dev);
> > > 
> > > This still has the issue of potentially letting exclusive reset control
> > > users break the device usage counter.
> > > 
> > > Also shared reset control users start with deassert(), and you end probe
> > > with pm_runtime_put(), so the first shared reset control user that
> > > deasserts its reset will decrement the dev->power.usage_count to -1 ?
> > > For multiple resets being initially deasserted this would decrement
> > > multiple times.
> > > 
> > > I think you'll have to track the (number of) asserted reset bits in this
> > > reset controller and limit when to call pm_runtime_get/put_sync().
> > > 
> > 
> > Yes, you're right.
> > 
> > I'll add a mask, and for each assert, the according bit will get set, and 
> > for each deasssert the same bit will get cleared.
> 
> > And when the mask has at least one bit set, the pm_runtime_get gets called
> 
> ^ When the mask was 0 before but now has a bit set.
> 
> > and when the mask is 0, the pm_runtime_put_sync will be called.
> 
> ^ When the mask had a bit set but now is 0.
> 
> > Does that sound OK ?
> 
> And the mask starts out as 0, as after the pm_runtime_put() in probe all
> reset lines are deasserted?
> 

Yes, that is correct.

> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int imx_blk_ctrl_reset_reset(struct reset_controller_dev *rcdev,
> > > > +					   unsigned long id)
> > > > +{
> > > > +	imx_blk_ctrl_reset_set(rcdev, id, true);
> > > > +	return imx_blk_ctrl_reset_set(rcdev, id, false);
> > > 
> > > Does this work for all peripherals? Are there none that require the
> > > reset line to be asserted for a certain number of bus clocks or similar?
> > 
> > As of now, there is no user that calls reset. All the users call the assert
> > and then deassert. As for the number of clocks for reset, I'll try to have a
> > chat to the HW design team and then come back with the information.
> 
> Ok. If this is not required or can't be guaranteed to work, it may be
> better to just leave it out.
> 
> regards
> Philipp