Re: [PATCH 07/14] clk: qcom: clk-branch: Add support for SREG branch ops

[Taniya Das]

On 10/18/2024 3:26 PM, Dmitry Baryshkov wrote:
> On Thu, Oct 17, 2024 at 03:28:13PM -0700, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2024-10-17 15:00:03)
> > > On Thu, Oct 17, 2024 at 11:10:20AM -0700, Stephen Boyd wrote:
> > > > Quoting Dmitry Baryshkov (2024-10-17 09:56:57)
> > > > > From: Kalpak Kawadkar <quic_kkawadka@xxxxxxxxxxx>
> > > > >
> > > > > Add support for SREG branch ops. This is for the clocks which require
> > > >
> > > > What is SREG? Can you spell it out?
> > >
> > > Unfortunately, no idea. This is the only register name I know.
> >
> > Can someone inside qcom tell us?
>
> Taniya, could you possibly help us? This is for gcc_video_axi0_sreg /
> gcc_video_axi1_sreg / gcc_iris_ss_hf_axi1_sreg /
> gcc_iris_ss_spd_axi1_sreg clocks on the SAR2130P platform.

SREG(Shift Register) are the register interface for clock branches which 
can control memories connected to them.

In principle these SREGs are not required to be controlled via SW 
interface, but on SAR2130P, we had to control them to flush out the 
pipeline for users of Video.

We are looking into the feasibility of extending the current 
'clk_branch2_mem_ops' and can share the patch.

You could also drop these clock interfaces for now to move ahead, as I 
do not see VideoCC support and bring them as part of those Clock 
controller support.

> > > > >          u8      halt_check;
> > > >
> > > > Instead of adding these new members can you wrap the struct in another
> > > > struct? There are usually a lot of branches in the system and this
> > > > bloats those structures when the members are never used.
> > > >
> > > >        struct clk_sreg_branch {
> > > >                u32 sreg_enable_reg;
> > > >                u32 sreg_core_ack_bit;
> > > >                u32 sreg_periph_ack_bit;
> > > >                struct clk_branch branch;
> > > >        };
> > > >
> > > > But I'm not even sure that is needed vs. just putting a clk_regmap
> > > > inside because the clk_ops don't seem to use any of these other members?
> > >
> > > Yes, nice idea. Is it ok to keep the _branch suffix or we'd better
> > > rename it dropping the _branch (and move to another source file while we
> > > are at it)?
> >
> > I don't really care. Inside qcom they called things branches in the
> > hardware and that name was carried into the code. If sreg is a branch
> > then that would make sense. From the 'core_ack' and 'periph_ack' it
> > actually looks like some sort of power switch masquerading as a clk.
>
> Ack.

--
Thanks & Regards,
Taniya Das.