Re: [PATCH v2 2/2] pmdomain: rockchip: Add support for rk3576 SoC

From: "Heiko Stübner" <heiko@sntech.de>
To: linux-kernel@vger.kernel.org,
	Detlev Casanova <detlev.casanova@collabora.com>
Cc: Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Finley Xiao <finley.xiao@rock-chips.com>,
	Jagan Teki <jagan@edgeble.ai>, Arnd Bergmann <arnd@arndb.de>,
	Elaine Zhang <zhangqing@rock-chips.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-pm@vger.kernel.org,
	kernel@collabora.com
Subject: Re: [PATCH v2 2/2] pmdomain: rockchip: Add support for rk3576 SoC
Date: Thu, 08 Aug 2024 19:59:59 +0200	[thread overview]
Message-ID: <2449940.aKNjEaI27c@diego> (raw)
In-Reply-To: <5805279.DvuYhMxLoT@trenzalore>

Hi Detlev,

Am Donnerstag, 8. August 2024, 19:53:20 CEST schrieb Detlev Casanova:
> On Thursday, 8 August 2024 12:41:05 EDT Heiko Stübner wrote:
> > > @@ -552,7 +575,10 @@ static int rockchip_pd_power(struct
> > > rockchip_pm_domain *pd, bool power_on)> 
> > >  			/* if powering up, leave idle mode */
> > >  			rockchip_pmu_set_idle_request(pd, false);
> > > 
> > > -			rockchip_pmu_restore_qos(pd);
> > > +			if (pd->info->delay_us)
> > > +				udelay(pd->info->delay_us);
> > > +			else
> > > +				rockchip_pmu_restore_qos(pd);
> > 
> > I still want this behaviour change in a separate patch with adequate
> > commit message please.
> > 
> > Going from always handling qos to allowing to just wait a specific time
> > needs explanation and is not part of "just" adding rk3576 support.
> 
> You are right, I didn't takle this issue.
> This is actually a bug, the else is not supposed to be there, it should only 
> be an added delay for some PDs.
> 
> Unfortunately, I'm not sure why that delay is needed exactly, so I'm willing 
> to remove it for now (only used by nputop and vop, both unsupported) and come 
> back to it if needed when VOP/NPU support is added.
> 
> Would that work for this upstream ?

that would work. The whole delay thing is not part of the dt-binding
which would be more critical to get right in the first round.

So as long as your "add rk3576 support" patch really only adds the
rk3576-specific data, but does not change how the shared code
behaves, we should be fine and can find out about that delay later.

Heiko