Re: [PATCH v2 11/17] clk: imx: Add blk_ctrl combo driver

[Abel Vesa <abel.vesa@nxp.com>]

On 20-08-25 14:07:29, Philipp Zabel wrote:
> On Tue, 2020-08-25 at 14:24 +0300, Abel Vesa wrote:
> [...]
> > > > +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;
> > > > +	unsigned int asserted_before = 0, asserted_after = 0;
> > > > +	u32 reg;
> > > > +	int i;
> > > > +
> > > > +	spin_lock_irqsave(&drvdata->lock, flags);
> > > > +
> > > > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > > +		if (drvdata->rst_hws[i].asserted)
> > > > +			asserted_before++;
> > > > +
> > > > +	if (asserted_before == 0 && assert)
> > > > +		pm_runtime_get(rcdev->dev);
> > > 
> > > Shouldn't that be pm_runtime_get_sync() ?
> > > 
> > > I would do that unconditionally before locking drvdata->lock and then
> > > drop unnecessary refcounts afterwards.
> > > 
> > 
> > I thought we already discussed this on the last's version thread.
> 
> This is about something different. pm_runtime_get() just queues the
> device to be enabled at a later point, but I presume you want to have it
> enabled before writing to its registers. (The question here is can you
> write to the registers, and have the device update its internal state,
> while the power domain is disabled?)
> Either way, if you want the reset to be asserted after the function
> returns (as is required by the reset API), as I understand it, you have
> to make sure that the power domain is activated before the function
> returns.
> Therefore pm_runtime_get_sync() is required instead of pm_runtime_get(),
> and that must be called outside of the spin locked section. My
> suggestion would be:
> 
> 	if (assert)
> 		pm_runtime_get_sync();
> 	spin_lock_irqsave();
> 	/* ... */
> 	spin_unlock_irqrestore();
> 	if (assert && asserted_before)
> 		pm_runtime_put();
> 

You're right this makes more sense.

> unless the following might be an issue:
> 
> > > > +
> > > > +	if (assert) {
> > > > +		reg = readl(reg_addr);
> > > > +		writel(reg & ~(mask << shift), reg_addr);
> > > > +		drvdata->rst_hws[id].asserted = true;
> > > > +	} else {
> > > > +		reg = readl(reg_addr);
> > > > +		writel(reg | (mask << shift), reg_addr);
> 
> Could this cause problems if the power domain is already disabled? If
> so, it would be best to either temporarily enable power, or to skip the
> register writes if asserted_before == 0 && !assert.

I'll go with the latter one since it leaves the PD off.

> 
> > > > +		drvdata->rst_hws[id].asserted = false;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> > > > +		if (drvdata->rst_hws[i].asserted)
> > > > +			asserted_after++;
> > > > +
> > > > +	if (asserted_before == 1 && asserted_after == 0)
> > > > +		pm_runtime_put(rcdev->dev);
> > > > +
> > > > +	spin_unlock_irqrestore(&drvdata->lock, flags);
> > > > +
> > > > +	return 0;
> > > > +}
> 
> regards
> Philipp