Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <6a4757e808c2201dc212d1ccf63310b3fd5b2268.camel@pengutronix.de>
Subject: Re: [PATCH v2 11/17] clk: imx: Add blk_ctrl combo driver
From:   Philipp Zabel <p.zabel@pengutronix.de>
To:     Abel Vesa <abel.vesa@nxp.com>
Cc:     Dong Aisheng <aisheng.dong@nxp.com>, Rob Herring <robh@kernel.org>,
        Peng Fan <peng.fan@nxp.com>, Fugang Duan <fugang.duan@nxp.com>,
        Jacky Bai <ping.bai@nxp.com>,
        Anson Huang <anson.huang@nxp.com>, devicetree@vger.kernel.org,
        Stephen Boyd <sboyd@kernel.org>,
        Mike Turquette <mturquette@baylibre.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        NXP Linux Team <linux-imx@nxp.com>,
        Sascha Hauer <kernel@pengutronix.de>,
        Fabio Estevam <fabio.estevam@nxp.com>,
        Shawn Guo <shawnguo@kernel.org>, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Date:   Wed, 26 Aug 2020 09:46:20 +0200
In-Reply-To: <20200825183009.olarh5ewkljeh35d@fsr-ub1664-175>
References: <1597406966-13740-1-git-send-email-abel.vesa@nxp.com>
         <1597406966-13740-12-git-send-email-abel.vesa@nxp.com>
         <ea2563fcb456830b37b0031455e5054d6b81c680.camel@pengutronix.de>
         <20200825112421.eut7gx3i4eirhnfw@fsr-ub1664-175>
         <8e8e33386eea12036bb17529b4d578704bf735d1.camel@pengutronix.de>
         <20200825183009.olarh5ewkljeh35d@fsr-ub1664-175>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.30.5-1.1 
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 2020-08-25 at 21:30 +0300, Abel Vesa wrote:
[...]
> > 	if (assert)
> > 		pm_runtime_get_sync();
> > 	spin_lock_irqsave();
> > 	/* ... */
> > 	spin_unlock_irqrestore();
> > 	if (assert && asserted_before)
> > 		pm_runtime_put();
> > 
> 
> On a second thought this doesn't work because, for the first assertion,
> the runtime put will never be called, if the asserted_before does not count
> the current assertion.

I'm not sure I follow. The first assert will increment device usage
0 -> 1, all others asserts will just temporarily increment and decrement
1 -> 2 -> 1. Isn't this just missing one
	if (!assert && !asserted_after)
		pm_runtime_put()
to do the last deassert 1 -> 0 transition?

> If it counts the current assertion, then every assertion
> will end with runtime put. None of these options work here.
>
> How about the following:
>
> 	if (assert && !test_and_set_bit(1, &drvdata->rst_hws[id].asserted))     
> 		pm_runtime_get_sync(rcdev->dev);                                
>                                                                         
> 	spin_lock_irqsave(&drvdata->lock, flags);                               
>                                                                         
> 	reg = readl(reg_addr);                                                  
> 	if (assert)                                                             
> 		writel(reg & ~(mask << shift), reg_addr);                       
> 	else                                                                    
> 		writel(reg | (mask << shift), reg_addr);                        
>                                                                         
> 	spin_unlock_irqrestore(&drvdata->lock, flags);                          
>                                                                         
> 	if (!assert && test_and_clear_bit(1, &drvdata->rst_hws[id].asserted))   
> 		pm_runtime_put(rcdev->dev);                                     
>                                                                         
> This would only call the get_sync/put once for each reset bit.

Yes, that should work. I think it is a much better idea, no more looping
through the entire reset control array.

regards
Philipp