Re: [PATCH v5 4/6] scsi: ufs: ufs-qcom: Switch to the new ICE API

[Eric Biggers]

On Fri, Apr 07, 2023 at 10:02:36AM +0300, Abel Vesa wrote:
> On 23-04-06 13:16:34, Eric Biggers wrote:
> > Hi Abel,
> > 
> > On Mon, Apr 03, 2023 at 11:05:28PM +0300, Abel Vesa wrote:
> > > Now that there is a new dedicated ICE driver, drop the ufs-qcom-ice and
> > > use the new ICE api provided by the Qualcomm soc driver ice. The platforms
> > > that already have ICE support will use the API as library since there will
> > > not be a devicetree node, but instead they have reg range. In this case,
> > > the of_qcom_ice_get will return an ICE instance created for the consumer's
> > > device. But if there are platforms that do not have ice reg in the
> > > consumer devicetree node and instead provide a dedicated ICE devicetree
> > > node, the of_qcom_ice_get will look up the device based on qcom,ice
> > > property and will get the ICE instance registered by the probe function
> > > of the ice driver.
> > > 
> > > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> > 
> > This is still silent about how the ICE clock behavior is being changed.
> 
> Right, I'll add the some more info into the commit message about the
> clock being handled by the ICE driver.
> 
> > 
> > I'm still trying to understand all this myself, so please bear with me, but my
> > understanding is that the UFS clocks can be disabled even while the host
> > controller is runtime-resumed.  This is called "clock gating" in the code.
> 
> The ICE clock is now being controlled by the new driver.
> > 
> > Before, the ICE clock was just listed as one of the UFS clocks.  So, it was just
> > managed like the other UFS clocks.
> > 
> > Now, it appears that the ICE clock is always enabled while the host controller
> > is runtime-resumed.  So, this patch removes support for gating of the ICE clock.
> 
> I just tested this and it works as expected, which is:
> 
> ICE clock gets enable on qcom_ice_create (via *clk_get*_enabled) and
> then, on the runtime suspend of the UFS, the qcom_ice_suspend is called
> which will disable the clock. Then, every time UFS runtime
> resumes/suspends the clock gets enabled/disabled.
> 
> Hope that makes sense.
> 
> Let me know if you think I'm missing something here.
> 

Well, it's better than v4 and earlier of this patchset, where the clock was
never turned off.

But, this patchset still seems to be a regression from the status quo, since it
makes the ICE clock no longer be disabled when "UFS clock gating" disables the
other UFS clocks.  Instead, it will only be disabled on runtime-suspend.

Now, I don't know whether anyone ever confirmed that the current behavior is
actually optimal and works as intended.  So, it *might* actually be fine to
change it!  But I was hoping that you at least had some thoughts about this,
whereas currently this patchset just ignores the issue entirely.

- Eric