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
