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

[Abel Vesa]

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