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
