Re: [PATCH V3 2/2] clk: scmi: support state_ctrl_forbidden

From: Cristian Marussi <cristian.marussi@arm.com>
To: Peng Fan <peng.fan@oss.nxp.com>
Cc: sudeep.holla@arm.com, mturquette@baylibre.com, sboyd@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, Peng Fan <peng.fan@nxp.com>
Subject: Re: [PATCH V3 2/2] clk: scmi: support state_ctrl_forbidden
Date: Sat, 20 Jan 2024 10:02:28 +0000	[thread overview]
Message-ID: <ZauaNDGTs8LFvnwT@pluto> (raw)
In-Reply-To: <c9385ba8-b225-4c8d-b09f-971d9e6008ab@oss.nxp.com>

On Sat, Jan 20, 2024 at 10:44:06AM +0800, Peng Fan wrote:
> 
> 
> 在 1/19/2024 2:27 AM, Cristian Marussi 写道:
> > On Mon, Jan 15, 2024 at 02:02:03PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > > 
> > > Some clocks may exported to linux, while those clocks are not allowed
> > > to configure by Linux. For example:
> > > 
> > > SYS_CLK1-----
> > >               \
> > > 	     --MUX--->MMC1_CLK
> > >               /
> > > SYS_CLK2-----
> > > 
> > > MMC1 needs set parent, so SYS_CLK1 and SYS_CLK2 are exported to Linux,
> > > then the clk propagation will touch SYS_CLK1 or SYS_CLK2.
> > > So we need bypass the failure for SYS_CLK1 or SYS_CLK2 when enable
> > > the clock of MMC1.
> > > 
> > 
> > Hi,
> > 
> > so this looks good to me and apparently (as noted) the CLK framework is OK
> > with a driver swallowing the -EACCESS when a clock is immutable, BUT at the
> > end of the day do we even need to try this SCMI call and hide the failure in
> > case of immutable clocks ?
> > 
> > I mean, what if we just dont provide any callback for enable/disable...I can
> > see plenty of drivers not providing those callbacks ?
> > Maybe this is probably more of a question for Stephen...
> > 
> > IOW what about doing something like below...does it make any difference
> > in your setup ? works fine in my emulated env
> 
> It should be fine to use your changes. Do you expect me to use your patch or
> make it as a follow up patch?
> 

It was just a suggestion, if you think is fine just include it in your
series, I dont mind.

> > 
> > (Note that last snippet in clk_gate_restore_context() is probably a fix
> >   that needs to be added anyway by looking at the code in clk.c)
> 
> This API seems only used by TI gate driver, this change should be in a
> standalone change go through clk tree. So I would your changes
> as a standalone optimization follow up patch, while not included
> in my patchset.

Yes, indeed I have made a small patch of it to post it separately..and I
forgot :D... sending it now.

Thanks,
Cristian